EthAccount tests for execute_from_outside_v2 - #22
Conversation
be409fb to
d736244
Compare
d955b49 to
fa4f3a2
Compare
d736244 to
71fe7d4
Compare
fa4f3a2 to
9dbe8b5
Compare
remollemo
left a comment
There was a problem hiding this comment.
@remollemo resolved 1 discussion.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
71fe7d4 to
fc412b7
Compare
9dbe8b5 to
6f25bda
Compare
fc412b7 to
f1aa2eb
Compare
6f25bda to
e49cf10
Compare
f1aa2eb to
a3f6531
Compare
e49cf10 to
7607417
Compare
a3f6531 to
da2eb8a
Compare
5fab790 to
6196e31
Compare
da2eb8a to
6badd88
Compare
6196e31 to
0c6ec15
Compare
0c6ec15 to
e833b00
Compare
|
reopen! |
remollemo
left a comment
There was a problem hiding this comment.
@remollemo reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
ItayRosenberg
left a comment
There was a problem hiding this comment.
@ItayRosenberg made 2 comments.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on remollemo).
eth_712_account/src/test_utils.cairo line 74 at r3 (raw file):
/// ANY_CALLER constant from SNIP-9. pub const ANY_CALLER: felt252 = 'ANY_CALLER';
r u testing the case where the caller is not any caller?
Code quote:
pub const ANY_CALLER: felt252 = 'ANY_CALLER';eth_712_account/src/test.cairo line 157 at r3 (raw file):
// Execute - this should succeed let _results = src9.execute_from_outside_v2(outside_execution, signature.span());
can u delete this?
Code quote:
let _results =
remollemo
left a comment
There was a problem hiding this comment.
@remollemo made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on ItayRosenberg).
eth_712_account/src/test_utils.cairo line 74 at r3 (raw file):
Previously, ItayRosenberg wrote…
r u testing the case where the caller is not any caller?
I tested this manually, will add this here too.
98fdc8a to
c2fbbc8
Compare
remollemo
left a comment
There was a problem hiding this comment.
@remollemo resolved 1 discussion.
Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on ItayRosenberg).
ItayRosenberg
left a comment
There was a problem hiding this comment.
@ItayRosenberg reviewed 5 files and made 9 comments.
Reviewable status: 5 of 6 files reviewed, 9 unresolved discussions (waiting on remollemo).
a discussion (no related file):
this what m agent wrote me
Python script nonce mismatch with test constants - The Python generator functions use hardcoded nonces (nonce=2 in generate_multi_call_test line 308, nonce=3 in generate_atomicity_test line 351) which don't match the Cairo constants (NONCE_MULTI_CALL=101, NONCE_ATOMICITY=102). The main() function overrides these after generation, so it works, but the intermediate generation is wasted. Consider using the nonce as a parameter from the start to avoid confusion.
a discussion (no related file):
re the duplication between the singatures in teh script and in the cairo
Two ways to improve:
-
Script writes directly to a generated file — instead of
print(), have the script output a.cairofile (e.g.,test_signatures.generated.cairo) that test_utils imports. No copy-paste needed. -
Single source of truth — the Python script already has all the constants (
EXPECTED_CONTRACT_ADDRESS, nonces, amounts). Buttest_utils.cairoalso defines them (NONCE_SINGLE_CALL,MOCK_ERC20_INITIAL_SUPPLY, etc.). If they drift apart, signatures silently break. The script should ideally read from a shared config or at least assert the constants match.
a discussion (no related file):
here (it is not 100% related)
earn-contracts/eth_712_account/src/eth_712_utils.cairo
Lines 18 to 19 in 59c6462
You ddint explain what is this (for the rest you did explain)
eth_712_account/src/test.cairo line 230 at r5 (raw file):
let outside_execution = get_test_outside_execution(); // Signature generated with EVM chain ID 2 - valid because contract uses chain_id from signature
im not sure that i understand this comment
Code quote:
// Signature generated with EVM chain ID 2 - valid because contract uses chain_id from signatureeth_712_account/src/test.cairo line 280 at r5 (raw file):
// ================================ // execute_from_outside_v2 tests with actual calls
test_execute_from_outside_specific_caller_succeeds
and test_execute_from_outside_wrong_caller_reverts should be before this
Code quote:
// execute_from_outside_v2 tests with actual callseth_712_account/src/test.cairo line 370 at r5 (raw file):
// Verify that the execution failed (error from second call propagates) assert!(result.is_err(), "Expected call to fail due to insufficient balance"); // In production, Starknet's protocol guarantees all state changes are rolled back.
cant we veirfy it?
I thought that the problem is only with events but maybe im wrong
Code quote:
// In production, Starknet's protocol guarantees all state changes are rolled back.eth_712_account/src/test.cairo line 371 at r5 (raw file):
assert!(result.is_err(), "Expected call to fail due to insufficient balance"); // In production, Starknet's protocol guarantees all state changes are rolled back. // snforge doesn't simulate this, so we cannot verify rollback here.
indent?
eth_712_account/src/test_utils.cairo line 93 at r5 (raw file):
pub fn setup_efo_test_with_erc20() -> (ISRC9_V2Dispatcher, ContractAddress, IERC20Dispatcher) { let src9 = setup_efo_test(); let (token_address, token) = deploy_mock_erc20(src9.contract_address);
why not using
deploy_mock_erc20_contract_at
eth_712_account/src/test_utils.cairo line 164 at r5 (raw file):
0x69b65ab69e218022a936846ea383df37, // s_low 27, // v 2 // hain_id: Not Ethereum
Suggestion:
chain_id: Not Ethereum
remollemo
left a comment
There was a problem hiding this comment.
@remollemo made 6 comments and resolved 2 discussions.
Reviewable status: 5 of 6 files reviewed, 7 unresolved discussions (waiting on ItayRosenberg).
a discussion (no related file):
Previously, ItayRosenberg wrote…
here (it is not 100% related)
earn-contracts/eth_712_account/src/eth_712_utils.cairo
Lines 18 to 19 in 59c6462
You ddint explain what is this (for the rest you did explain)
what's to explain? that's the hash
eth_712_account/src/test.cairo line 230 at r5 (raw file):
Previously, ItayRosenberg wrote…
im not sure that i understand this comment
it's for the mismatching chain id test
eth_712_account/src/test.cairo line 280 at r5 (raw file):
Previously, ItayRosenberg wrote…
test_execute_from_outside_specific_caller_succeeds
andtest_execute_from_outside_wrong_caller_revertsshould be before this
i don't understand
eth_712_account/src/test.cairo line 370 at r5 (raw file):
Previously, ItayRosenberg wrote…
cant we veirfy it?
I thought that the problem is only with events but maybe im wrong
it doesn't work
eth_712_account/src/test_utils.cairo line 93 at r5 (raw file):
Previously, ItayRosenberg wrote…
why not using
deploy_mock_erc20_contract_at
what's wrong with this one?
eth_712_account/src/test_utils.cairo line 164 at r5 (raw file):
0x69b65ab69e218022a936846ea383df37, // s_low 27, // v 2 // hain_id: Not Ethereum
:-)
remollemo
left a comment
There was a problem hiding this comment.
@remollemo made 2 comments and resolved 1 discussion.
Reviewable status: 5 of 6 files reviewed, 6 unresolved discussions (waiting on ItayRosenberg).
a discussion (no related file):
Previously, ItayRosenberg wrote…
this what m agent wrote me
Python script nonce mismatch with test constants - The Python generator functions use hardcoded nonces (nonce=2 in generate_multi_call_test line 308, nonce=3 in generate_atomicity_test line 351) which don't match the Cairo constants (NONCE_MULTI_CALL=101, NONCE_ATOMICITY=102). The main() function overrides these after generation, so it works, but the intermediate generation is wasted. Consider using the nonce as a parameter from the start to avoid confusion.
I don't care what your agent told. it's you agent.
a discussion (no related file):
Previously, ItayRosenberg wrote…
re the duplication between the singatures in teh script and in the cairo
Two ways to improve:
Script writes directly to a generated file — instead of
print(), have the script output a.cairofile (e.g.,test_signatures.generated.cairo) that test_utils imports. No copy-paste needed.Single source of truth — the Python script already has all the constants (
EXPECTED_CONTRACT_ADDRESS, nonces, amounts). Buttest_utils.cairoalso defines them (NONCE_SINGLE_CALL,MOCK_ERC20_INITIAL_SUPPLY, etc.). If they drift apart, signatures silently break. The script should ideally read from a shared config or at least assert the constants match.
I will fix the python scripts etc. in next PR (the one with the normal tx support)
c2fbbc8 to
4210bee
Compare
remollemo
left a comment
There was a problem hiding this comment.
@remollemo made 1 comment and resolved 5 discussions.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on ItayRosenberg).
eth_712_account/src/test.cairo line 280 at r5 (raw file):
Previously, remollemo wrote…
i don't understand
I moved it a bit.
remollemo
left a comment
There was a problem hiding this comment.
@remollemo resolved 1 discussion.
Reviewable status: 4 of 6 files reviewed, all discussions resolved (waiting on ItayRosenberg).
4210bee to
4484090
Compare
remollemo
left a comment
There was a problem hiding this comment.
@remollemo partially reviewed 1 file.
Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on ItayRosenberg).
ItayRosenberg
left a comment
There was a problem hiding this comment.
@ItayRosenberg reviewed 3 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on remollemo).
a discussion (no related file):
Previously, remollemo wrote…
what's to explain? that's the hash
all the rest are with explanations
eth_712_account/src/test_utils.cairo line 93 at r5 (raw file):
Previously, remollemo wrote…
what's wrong with this one?
because it is already ready in the test_utils
so less code dup
a discussion (no related file):
missing test - INVALID_SIGNATURE_LENGTH
remollemo
left a comment
There was a problem hiding this comment.
@remollemo made 3 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ItayRosenberg).
a discussion (no related file):
in #23
a discussion (no related file):
Previously, ItayRosenberg wrote…
missing test -
INVALID_SIGNATURE_LENGTH
in #23
eth_712_account/src/test_utils.cairo line 93 at r5 (raw file):
Previously, ItayRosenberg wrote…
because it is already ready in the test_utils
so less code dup
changed (and using deploy_at)

Note
Low Risk
Primarily adds/updates tests, fixtures, and build/test tooling; no production contract logic changes, with the main risk being test brittleness due to hardcoded deterministic addresses/signatures.
Overview
Adds an end-to-end test suite for
execute_from_outside_v2, covering success paths and expected reverts for time bounds, nonce replay, invalid signatures, domain-separator mismatches (EVM chain id, Starknet chain name, verifying contract), and caller restrictions.Extends test utilities with EFO setup helpers (timestamp/chain id cheats),
OutsideExecutionbuilders, precomputed EIP-712 signatures, and an externalDualCaseERC20Mockdeployment to validate real approve/transfer multi-call behavior (including failure propagation via the safe dispatcher). Also introduces a Python signature generator (scripts/generate_test_signatures.py+requirements.txt) and updates Scarb config/lockfile to includestarkware_utilsand build the external mock contract for tests.Written by Cursor Bugbot for commit 4484090. This will update automatically on new commits. Configure here.
This change is