test(e2e-evm): Add message call tests #2034
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Initial set of tests that just verify the use of callcode. This is a prerequisite set of tests that ensure our Caller contract uses callcode as expected, as the ABI compliance tests do not validate this behavior.
Build the whole tx params in the test cases themselves to be more explicit and more flexible.
Validate behavior of callcode when storage is used, primarily where the storage is set.
This ensures value was transferred in the tests, which paired with the msg.value checks, shows that msg.value is not preserved in callcode.
Expecting 0 value would make the conditional fail with a falsey value. Adding additional else case to ensure the return value is undefined as an extra check in case the assertions are not being run. This correctly validates msg.value does indeed have the value passed to callcode(), just not the same one as msg.value of the parent caller.
nddeluca
reviewed
Oct 17, 2024
| @@ -170,16 +170,29 @@ describe("ABI_BasicTests", function () { | |||
| { | |||
| name: "can be called by low level contract call", | |||
| txParams: (ctx) => ({ | |||
| to: caller.address, | |||
| to: lowLevelCaller.address, | |||
There was a problem hiding this comment.
Execellent rename 👍. I was a bit lazy with the initial one -- lowLevelCaller gives better clarity on what this test is doing
nddeluca
reviewed
Oct 17, 2024
tests/e2e-evm/test/callcode.test.ts
Outdated
| // | ||
| let publicClient: PublicClient; | ||
| let walletClient: WalletClient; | ||
| let lowLevelCaller: GetContractReturnType<ArtifactsMap["Caller"]["abi"]>; |
nddeluca
reviewed
Oct 17, 2024
tests/e2e-evm/test/callcode.test.ts
Outdated
| // callCodeTestCaseStorage is for setStorageValue() call tests to validate | ||
| // which contract the storage is set on and the expected storage value | ||
| type callCodeTestCaseStorage = callCodeTestCaseBase & { | ||
| wantSender?: never; |
nddeluca
reviewed
Oct 17, 2024
tests/e2e-evm/test/callcode.test.ts
Outdated
| wantStorageValue: bigint; | ||
| }; | ||
|
|
||
| type callCodeTestCase = callCodeTestCaseSendAndValue | callCodeTestCaseStorage; |
There was a problem hiding this comment.
This is quite a nice method for adding multiple kinds of tests in the same suite, thanks for teaching me this ✍️
There was a problem hiding this comment.
The want* prefix is surprising nice to read
nddeluca
reviewed
Oct 17, 2024
| functionName: "functionCall", | ||
| args: [ctx.address, funcSelector], | ||
| }), | ||
| gas: contractCallerGas, | ||
| }), | ||
| expectedStatus: "success", | ||
| }, | ||
| { |
nddeluca
reviewed
Oct 17, 2024
tests/e2e-evm/test/callcode.test.ts
Outdated
| txData.account = whaleAddress; | ||
|
|
||
| if (!txData.to) { | ||
| expect.fail("to field not set"); |
There was a problem hiding this comment.
Nice type assertions to guard against errors in the test cases
nddeluca
reviewed
Oct 17, 2024
tests/e2e-evm/test/callcode.test.ts
Outdated
| // Storage tests if applicable | ||
| if (tc.wantStorageContract) { | ||
| const storageContract = tc.wantStorageContract(ctx); | ||
| const storageValue = await publicClient.getStorageAt({ |
There was a problem hiding this comment.
Good use of the storage API when the calling contract doesn't have a function to retrieve the value via call
nddeluca
reviewed
Oct 17, 2024
| @@ -18,12 +18,67 @@ contract Caller { | |||
|
|
|||
| // solhint-disable-next-line no-inline-assembly | |||
| assembly { | |||
| // Bubble up errors: revert(pointer, length of revert reason) | |||
There was a problem hiding this comment.
Thank you for adding comments where I should have left them 🥇
nddeluca
reviewed
Oct 17, 2024
| function functionCallCode(address to, bytes calldata data) external payable { | ||
| // solhint-disable-next-line no-inline-assembly | ||
| assembly { | ||
| // Copy the calldata to memory, as callcode uses memory pointers. |
There was a problem hiding this comment.
💯 and good note on memory safety for future reference if modifying this function
This restructures the tests to confirm the expected msg.value and msg.sender with various different message call types, e.g. call, delegatecall, callcode, staticcall. The storage tests are also split up so that they are independent set of tests to keep different types of tests separate and reduce clutter with the slight increase in redundancy in the test logic.
With the previous changes of functionCallCode calling the functionCallCodeWithValue cause an increase in gas usage since calling an function marked as external consume more gas in addition to the overhead of calling anothher function. This function was added to allow more control over the value the caller contract sends with the message call to the implementation contract.
It now contains general message context and storage for different call types
This also resolves the no-unused-expressions lint errors
Prerequisite changes to allow for testing the same set of tests against the precompile versions of the contracts. This follows the similar pattern as the ABI tests where the test context can be initialized by dynamic values for the implementation contract.
This implements shared ABI interfaces for both the mock contracts and later on, precompiles, that are used for the tests. The main goal is to test precompiles handle different message call types correctly, msg.sender, msg.value, storage location. This expands the current set of tests that simply define the behavior of call message types to a set of tests that can also enforce the same behavior on a set of precompiles.
STATICCALL is expected to revert for storage tests, so having a expected storage owner is misleading. This makes it optional so we can show in the test cases that there is no expected storage.
drklee3
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
functionCallCodeto low level caller contractfunctionCallCodeis correct and context behaviormsg.sender,msg.value, and storage locationcallcodeuse for ABI basic tests to call mock contracts.