Skip to content

eth-712-account support privacy - #26

Merged
remollemo merged 2 commits into
mainfrom
remo/exp-support-privacy
Jun 28, 2026
Merged

eth-712-account support privacy#26
remollemo merged 2 commits into
mainfrom
remo/exp-support-privacy

Conversation

@remollemo

@remollemo remollemo commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

This change is Reviewable

@remollemo remollemo self-assigned this Jun 17, 2026
@remollemo
remollemo requested a review from ItayRosenberg June 17, 2026 08:19
@remollemo
remollemo force-pushed the remo/exp-support-privacy branch 2 times, most recently from adf8669 to 00c2c0a Compare June 18, 2026 12:27
@remollemo
remollemo force-pushed the remo/exp-support-privacy branch from 00c2c0a to b44fe1e Compare June 25, 2026 07:49
@remollemo
remollemo merged commit b44fe1e into main Jun 28, 2026
2 of 3 checks passed
@remollemo
remollemo deleted the remo/exp-support-privacy branch June 28, 2026 09:16

@ItayRosenberg ItayRosenberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ItayRosenberg reviewed 4 files and made 5 comments.
Reviewable status: 4 of 8 files reviewed, 5 unresolved discussions.


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

        }

        fn get_account(self: @ContractState, eth_address: EthAddress) -> ContractAddress {

it is confusing that one is get_account and the other get..._address

Code quote:

 fn get_account(self: @ContractState, eth_address: EthAddress) -> ContractAddre

account_factory/src/account_factory.cairo line 118 at r2 (raw file):

        fn get_account(self: @ContractState, eth_address: EthAddress) -> ContractAddress {
            let account_address = eth_address_to_account(:eth_address);

why not use get\_expected\_account\_address?

Code quote:

 eth_address_to_account(:eth_address);

eth_712_account/scripts/eip712.py line 206 at r2 (raw file):

def hash_call_set(calls: list[dict]) -> int:
    """Hash CallSet struct matching call_set_hash_struct in eth_712_utils.cairo."""
    return keccak_ints(CALL_SET_TYPE_HASH, hash_call_array(calls))

Suggestion:

:calls)

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

            self: @ContractState, calls: Span<Call>, signature: Span<felt252>,
        ) -> felt252 {
            let (signature, evm_chain_id) = extract_signature(signature);

Suggestion:

(:signature);

eth_712_account/src/interface.cairo line 18 at r2 (raw file):

/// Returns `VALIDATED` for a valid 6-felt signature and `0` for an invalid one
/// Reverts on a malformed signature.
/// signature (wrong length or out-of-range components) reverts.
    /// Returns `VALIDATED` for a valid 6-felt signature and 0 for an invalid one.
    /// Reverts on a malformed signature.

Code quote:

/// Returns `VALIDATED` for a valid 6-felt signature and `0` for an invalid one
/// Reverts on a malformed signature.
/// signature (wrong length or out-of-range components) reverts.

@remollemo remollemo left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@remollemo made 2 comments and resolved 5 discussions.
Reviewable status: 4 of 8 files reviewed, all discussions resolved.


eth_712_account/scripts/eip712.py line 206 at r2 (raw file):

def hash_call_set(calls: list[dict]) -> int:
    """Hash CallSet struct matching call_set_hash_struct in eth_712_utils.cairo."""
    return keccak_ints(CALL_SET_TYPE_HASH, hash_call_array(calls))
  1. the added value of named variables in a single arg is arguable.
  2. it's python, not cairo.

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

            self: @ContractState, calls: Span<Call>, signature: Span<felt252>,
        ) -> felt252 {
            let (signature, evm_chain_id) = extract_signature(signature);
  1. the added value of named variables in a single arg is arguable.
  2. we have a few cases, 1 with :signature and a few without.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants