Validate new public key in Account

[ericnordelo]

Fixes #818

PR Checklist

• Tests
• Documentation
• Added entry to CHANGELOG.md
• Tried the feature on a public network

[andrew-fleming]

This looks very good, Eric! I left a couple questions on the proposed message hash construction and a few minor suggestions

[andrew-fleming]

Should we include the domain separator in compliance with SNIP-12?

[andrew-fleming]

And I assume you don't think it's worth it to include the account and the new owner in the message as Marto initially proposed?

#[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.

[ericnordelo]

For this particular case, I don't think SNIP12 is suited, because we don't really need to sign a struct with nonces and so on, we just need to verify that the signer exists, and for that something like EIP 191 (version 0x45) should suffice. If we were to be SNIP 12 compliant, we would need to add a domain separator, with SNIP12Metadata, and that seems like an overhead for the use case.

The proposed format is to avoid clashes with transactions by prefixing "StarkNet Message", and accept_ownership to scope the message, and then we have the account address and the previous owner, so the user is signing who will be receiving the ownership from. To me adding the new owner to the signature is redundant since the signature already enforces the new pub key.

[ericnordelo]

Notice that, the use case is to avoid transferring the ownership to an invalid public key.

[andrew-fleming]

something like EIP 191 (version 0x45) should suffice. If we were to be SNIP 12 compliant, we would need to add a domain separator, with SNIP12Metadata, and that seems like an overhead for the use case.

To me adding the new owner to the signature is redundant since the signature already enforces the new pub key.

Good points!

[andrew-fleming]

LGTM!