test(e2e-evm): Add message call tests#2034
Open
drklee3 wants to merge 30 commits into
Open
Conversation
Fallback tests are not run on each function but rather only for the specific fallback. This means whether or not to run the test is determined by only the receive and fallback functions, without any other ABI function.
Previously used type assertions to bypass certain TypeScript issues with test cases, along with using any & unsafe assignments. This resolves the types to be properly valid and enforced to prevent any potential errors.
Changes from including a field in each test case from conditionally running the case, to building the cases dynamically. This allows for logical grouping of test cases and organization with logic instead of using comments. Slightly less explicit for each test case, but with the grouping of test cases, it reduces the mental overhead of figuring out when each test case is run.
Resolves use type casting and unsafe access, validation of revert errors for matches and types
Previously runs all the time, which is currently okay with the current single testing contract that includes both functions. This conditionally adds these test cases if the respective functions exist so we can test additional contract behavior that may not have these functions and may produce a different error.
Most of these issues are intentional and are okay to ignore. This also sets the solhint ignoreConstructors option to true for the func-visibility rule, as we are using solidity >=0.7.0
Long errors are truncated and difficult to determine the issue otherwise
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
| name: "can be called by low level contract call", | ||
| txParams: (ctx) => ({ | ||
| to: caller.address, | ||
| to: lowLevelCaller.address, |
Member
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
| // | ||
| let publicClient: PublicClient; | ||
| let walletClient: WalletClient; | ||
| let lowLevelCaller: GetContractReturnType<ArtifactsMap["Caller"]["abi"]>; |
nddeluca
reviewed
Oct 17, 2024
| // 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
| wantStorageValue: bigint; | ||
| }; | ||
|
|
||
| type callCodeTestCase = callCodeTestCaseSendAndValue | callCodeTestCaseStorage; |
Member
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 ✍️
Member
There was a problem hiding this comment.
The want* prefix is surprising nice to read
nddeluca
reviewed
Oct 17, 2024
| }), | ||
| expectedStatus: "success", | ||
| }, | ||
| { |
nddeluca
reviewed
Oct 17, 2024
| txData.account = whaleAddress; | ||
|
|
||
| if (!txData.to) { | ||
| expect.fail("to field not set"); |
Member
There was a problem hiding this comment.
Nice type assertions to guard against errors in the test cases
nddeluca
reviewed
Oct 17, 2024
| // Storage tests if applicable | ||
| if (tc.wantStorageContract) { | ||
| const storageContract = tc.wantStorageContract(ctx); | ||
| const storageValue = await publicClient.getStorageAt({ |
Member
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
|
|
||
| // solhint-disable-next-line no-inline-assembly | ||
| assembly { | ||
| // Bubble up errors: revert(pointer, length of revert reason) |
Member
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. |
Member
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
nddeluca
requested review from
karzak,
pirtleshell and
rhuairahrighairidh
as code owners
Base automatically changed from
dl-e2e-abi-tests-refactor
to
nd-implement-basic-precompile-calling
May 3, 2025 13:37
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.