Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Storage test #60

Merged
merged 6 commits into from
Dec 10, 2023
Merged

Add Storage test #60

merged 6 commits into from
Dec 10, 2023

Conversation

ephess
Copy link
Contributor

@ephess ephess commented Nov 30, 2023

No description provided.

@BenSparksCode
Copy link
Contributor

This looks solid! Nice use of Foundry's libs to find those storage slots.

The only thing I think we should add are checks of values set in the constructor - you have most of them, just missing INITIAL_CHAIN_ID and INITIAL_DOMAIN_SEPARATOR.

Because they're internal, you'll have to make a "dummy" contract at the bottom of the test file that inherits Storage, and then includes public functions you can call which return the values of those internal variables. You'll also need to implement _computeDomainSeparator() but maybe just make it return bytes32("SEPARATOR") and test that the var gets set correctly.

Here's an example of this inheritance thing to expose the internal functions of a contract for a test:
https://github.com/FastLane-Labs/atlas/blob/main/test/Permit69.t.sol#L260

@ephess
Copy link
Contributor Author

ephess commented Dec 8, 2023

Thanks Ben, I've implemented your suggestions. Appreciate the feedback.

Any thoughts on how to target the storage slots I haven't been able to test with the stdstore.target stuff? It's a bit confusing to me to try figure out how to chain these calls for more complex data structures. Perhaps it's more obvious to you.

I was planning on just leaving this as a TODO for a future punter.

@BenSparksCode
Copy link
Contributor

Any thoughts on how to target the storage slots [...] for more complex data structures.

Yeah, it's very involved when you get into more complex data structures (combinations of mappings, structs, arrays).

I think we can leave that out for this PR and test file. The only reason I can think of to test around that sort of thing is to make sure that the caller and callee contracts in a delegatecall tx have the same storage layout so the correct data is targeted. But we'll do some tests that cover that in the parts of Atlas that delegatecall the lib contracts.
So I think this is great to merge as is. Good job!

@ephess ephess merged commit 30a2016 into main Dec 10, 2023
3 checks passed
@ephess ephess deleted the add-storage-test branch December 10, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants