EthAccount tests for initialization & upgrade

[remollemo]

Note

Low Risk
Mostly adds test-only code and dev-dependency wiring; no production logic changes beyond exposing/using existing upgrade behavior in tests.

Overview
Adds snforge tests for StarknetEth712Account covering initialize success and revert paths, plus upgrade authorization, Upgraded event emission, and optional EIC-driven interface registration.

Wires in new test modules (test.cairo, test_utils.cairo) and updates eth_712_account dev-dependencies/lockfile to include starkware_utils_testing and local testing_utils used for caller spoofing and event assertions.

^{Written by Cursor Bugbot for commit 56ec34e. This will update automatically on new commits. Configure here.}

---

This change is 

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

[remollemo]

@remollemo resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @remollemo).

[ItayRosenberg]

@ItayRosenberg reviewed 3 files and made 12 comments.
Reviewable status: all files reviewed (commit messages unreviewed), 12 unresolved discussions (waiting on @remollemo).

---

a discussion (no related file):
general question about the upgrade
is it ok that someone will add eic without upgrading?

---

a discussion (no related file):
im not sure what is the difference between test_initialize_invalid_signature_reverts and test_initialize_wrong_address_reverts

this is over engineering i think

---

eth_712_account/src/test.cairo line 16 at r2 (raw file):

#[test]
fn test_initialize_success() {

Doesn't verify that eth_address was actually stored correctly. Test could pass even if address storage is broken (as long as interfaces register).

---

eth_712_account/src/test.cairo line 76 at r2 (raw file):

    // Try to upgrade from external caller (not self)
    let new_class_hash = crate::test_utils::declare_eth712_account();

are you ok with this??
why not import it

Code quote:

 let new_class_hash = crate::test_utils::declare_eth712_account();

---

eth_712_account/src/test.cairo line 76 at r2 (raw file):

    // Try to upgrade from external caller (not self)
    let new_class_hash = crate::test_utils::declare_eth712_account();

blocking :(
This already happens as part of the deploy command

/// Declare and deploy the StarknetEth712Account contract.
pub fn deploy_eth712_account() -> ContractAddress {
    let contract_class = snforge_std::declare("StarknetEth712Account")
        .unwrap_syscall()
        .contract_class();
    let (contract_address, _) = contract_class.deploy(@array![]).unwrap_syscall();
    contract_address
}

1. you should probably reuse in the deploy the declare function
2. there is no need to do this!

---

eth_712_account/src/test.cairo line 83 at r2 (raw file):

fn test_upgrade_from_self_succeeds() {
    let contract_address = deploy_eth712_account();
    let dispatcher = IAccount712AdminDispatcher { contract_address };

this is not an informative name, sorry this is what you always says to me

Code quote:

 let dispatcher 

---

eth_712_account/src/test.cairo line 89 at r2 (raw file):

    // Spoof caller as self
    start_cheat_caller_address(contract_address, contract_address);

why not cheat_caller_address_once?

---

eth_712_account/src/test.cairo line 103 at r2 (raw file):

    assert!(*from == contract_address, "Event from wrong address");
    assert!(event.data.len() > 0, "Event has no data");
    assert!(*event.data.at(0) == new_class_hash.into(), "Wrong class hash in event");

let (from, event) = *get_event_by_selector(events.events, selector!("Upgraded"))
    .expect('Upgraded event not found');
    
assert!(*from == contract_address, "Event from wrong address");
assert!(*event.data.at(0) == new_class_hash.into(), "Wrong class hash in event");

Code quote:

    assert!(events.events.len() > 0, "No events emitted");

    // Verify the event data contains the new class hash
    let (from, event) = events.events.at(0);
    assert!(*from == contract_address, "Event from wrong address");
    assert!(event.data.len() > 0, "Event has no data");
    assert!(*event.data.at(0) == new_class_hash.into(), "Wrong class hash in event");

---

eth_712_account/src/test.cairo line 105 at r2 (raw file):

    assert!(*event.data.at(0) == new_class_hash.into(), "Wrong class hash in event");

    stop_cheat_caller_address(contract_address);

why do we need this?

Code quote:

    stop_cheat_caller_address(contract_address);

---

eth_712_account/src/test.cairo line 117 at r2 (raw file):

    // Spoof caller as self
    start_cheat_caller_address(contract_address, contract_address);

why not
cheat_caller_address_once

---

eth_712_account/src/test.cairo line 132 at r2 (raw file):

    assert!(src5.supports_interface(custom_interface_id), "Custom interface not registered");

    stop_cheat_caller_address(contract_address);

why do we need this?

Code quote:

   stop_cheat_caller_address(contract_address);

---

eth_712_account/src/test_utils.cairo line 63 at r2 (raw file):

pub fn declare_register_interfaces_eic() -> ClassHash {
    *snforge_std::declare("RegisterInterfacesEIC").unwrap_syscall().contract_class().class_hash
}

u should use it directly in the test
no need for a test utils function for a one line in one place in a test

Code quote:

/// Declare the RegisterInterfacesEIC contract and return its class hash.
pub fn declare_register_interfaces_eic() -> ClassHash {
    *snforge_std::declare("RegisterInterfacesEIC").unwrap_syscall().contract_class().class_hash
}

[remollemo]

@remollemo made 8 comments and resolved 8 discussions.
Reviewable status: 1 of 6 files reviewed, 4 unresolved discussions (waiting on @ItayRosenberg and @remollemo).

---

a discussion (no related file):

Previously, ItayRosenberg wrote…

general question about the upgrade
is it ok that someone will add eic without upgrading?

i don't understand the question. what does it mean "to add an eic"?

---

a discussion (no related file):

Previously, ItayRosenberg wrote…

im not sure what is the difference between test_initialize_invalid_signature_reverts and test_initialize_wrong_address_reverts

this is over engineering i think

there is no extra engineering, so it cannot be "over engineering".
one (you, maybe) can claim that altering any of the two is equivalent. thereby making the two tests effectively equivalent, i.e. redundunt.

---

eth_712_account/src/test.cairo line 16 at r2 (raw file):

Previously, ItayRosenberg wrote…

Doesn't verify that eth_address was actually stored correctly. Test could pass even if address storage is broken (as long as interfaces register).

i will add this.

---

eth_712_account/src/test.cairo line 76 at r2 (raw file):

Previously, ItayRosenberg wrote…

are you ok with this??
why not import it

i don't really care, but i prefer in the imports too.

---

eth_712_account/src/test.cairo line 76 at r2 (raw file):

Previously, ItayRosenberg wrote…

blocking :(
This already happens as part of the deploy command

/// Declare and deploy the StarknetEth712Account contract.
pub fn deploy_eth712_account() -> ContractAddress {
    let contract_class = snforge_std::declare("StarknetEth712Account")
        .unwrap_syscall()
        .contract_class();
    let (contract_address, _) = contract_class.deploy(@array![]).unwrap_syscall();
    contract_address
}

1. you should probably reuse in the deploy the declare function
2. there is no need to do this!

1. the fact that delcare is done in the deploy is irrelevant (actually, it need not be done there either)
2. indeed, there is no need to declare here to test for upgrade right.
   al ze lahsom??? the test is doing exactly what it needs to do, even if in a non-optimized manner. as the declare is running locally and not on a system, there is no real issue with that.

---

eth_712_account/src/test.cairo line 103 at r2 (raw file):

Previously, ItayRosenberg wrote…

let (from, event) = *get_event_by_selector(events.events, selector!("Upgraded"))
    .expect('Upgraded event not found');
    
assert!(*from == contract_address, "Event from wrong address");
assert!(*event.data.at(0) == new_class_hash.into(), "Wrong class hash in event");

it's hard to understand your comments this way.

---

eth_712_account/src/test.cairo line 105 at r2 (raw file):

Previously, ItayRosenberg wrote…

why do we need this?

it's the stopper for the start_cheat. i will tell him to use once, which is enough here

---

eth_712_account/src/test_utils.cairo line 63 at r2 (raw file):

Previously, ItayRosenberg wrote…

u should use it directly in the test
no need for a test utils function for a one line in one place in a test

it's ok this way too.

[remollemo]

@remollemo resolved 2 discussions.
Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @ItayRosenberg).

[remollemo]

@remollemo resolved 1 discussion.
Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @ItayRosenberg).

[ItayRosenberg]

@ItayRosenberg reviewed 4 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on remollemo).

---

eth_712_account/src/test.cairo line 103 at r2 (raw file):

Previously, remollemo wrote…

it's hard to understand your comments this way.

i said that you can change it to be much more rreable
This works (i checked)

    // Verify Upgraded event was emitted
    let (from, event) = get_event_by_selector(
        spy.get_events().events.span(),
        selector!("Upgraded")
    ).expect('Upgraded event not found');

    assert!(*from == account_address, "Event from wrong address");
    assert!(*event.data.at(0) == class_hash.into(), "Wrong class hash in event");