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 a delegateCall test regarding memory #25

Merged
merged 3 commits into from
Aug 8, 2022

Conversation

AtomicAzzaz
Copy link
Contributor

This test asserts that the delegateCall function of the delegateCall library leaves the memory in the same state as a the targetDelegateCallBehaviour function - which was the first implementation of the delegateCall library before being optimized in yul. This test compares the free memory pointer value and the full memory state using both functions.

The idea behind targetDelegateCallBehaviourAndReturnMemoryAndPointer is to write in the memory before and after the delegateCall to not be in a trivial state with only 0's in memory. We test the memory on various data types returned by the delegateCall.

Note: it is possible to make a full working implementation having this test failing (like having the free mem pointer further than ref implem) but is I believe not safe to do.

Copy link
Contributor

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useful!
Could we extract targetDelegateCallBehaviour to a DelegateCallRef contract?
To keep the same architecture as for the other utils

Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small changes to apply but overall LGTM

test/TestDelegateCall.sol Outdated Show resolved Hide resolved
test/TestDelegateCall.sol Outdated Show resolved Hide resolved
test/TestDelegateCall.sol Outdated Show resolved Hide resolved
test/TestDelegateCall.sol Outdated Show resolved Hide resolved
Signed-off-by: Merlin <44097430+MerlinEgalite@users.noreply.github.com>
@MerlinEgalite MerlinEgalite merged commit f32ca41 into main Aug 8, 2022
@Rubilmax Rubilmax deleted the feat/delegateCallTestMemory branch December 13, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants