Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify signer control on setPublicKey #922

Closed
wants to merge 1 commit into from
Closed

Conversation

martriay
Copy link
Contributor

@martriay martriay commented Feb 22, 2024

Fixes #469, #818.

I propose not adding a 2step flow, but to add a signature: Span<felt252> param to setPublicKey (*) used to validate that we control the new owner, and that it "accepts" this ownership -- all in a single step.

To make this more user-friendly, i'm using SNIP-12 (aka Starknet's EIP712) and defining the AddOwner operation for the Account.add_owner application.

#[derive(Copy, Drop, Serde)]
struct AddOwner {
    account: ContractAddress,
    owner: felt252
}

Even though the account is already present in the hash as required by SNIP-12 and that owner is already known by the owner, I decided to make the struct overly explicit so users know what they're signing, even if it adds a bit of cost/redundancy/complexity.

If we move forward with this proposal, we should probably tackle #409 first.

*: i'd probably go even further an rename it set_owner, since that's the term we're using in the OwnerAdded and that I'm using here too.

@martriay martriay marked this pull request as draft February 22, 2024 02:41
@martriay
Copy link
Contributor Author

Maybe it's worth considering having a 2step mechanism anyway, since in the current design the flow requires a bit of coordination between old and new owner:

  1. new owner signs acceptance message
  2. new owner sends it to old owner
  3. old owner crafts and sends the tx to the account

While if it's two step:

  1. former owner proposes new owner
  2. new owner accepts ownership

@martriay martriay linked an issue Mar 4, 2024 that may be closed by this pull request
@ericnordelo
Copy link
Member

Closing in favor of #989

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants