CIP10 contracts changes #5913
CIP10 contracts changes #5913
Conversation
AlexBHarley
force-pushed
the
alexbharley/cip10
branch
from
51aa6c8
to
53b8cac
Compare
AlexBHarley
force-pushed
the
alexbharley/cip10
branch
from
53b8cac
to
fdb3ea3
Compare
| * @param _account The address of account that authorized signing. | ||
| * @param role The role to finish authorizing for. | ||
| */ | ||
| function completeSignerAuthorization(address _account, string memory role) public { |
There was a problem hiding this comment.
maybe I'm not familiar enough with CIP10 but why separate authorize and complete, and why limit this to only signer == msg.sender?
what is the downside to someone authorizing and completing you as a signer on your behalf?
There was a problem hiding this comment.
It's a double opt-in. Like we have for our existing roles. You don't really want to allow the "burning" of an address in this scheme without their consent
There was a problem hiding this comment.
Agree that it's a little confusing. I guess there's two things at play here
- the act of proving you own an address
- setting that address as an authorised signer for an account.
Our system treats those two as one step currently. One question to ask is why have authorizeSignerWithSignature at all? The answer being we want to prove proof of possession before saying XXX was authorised by YYY.
Having completeSignerAuthorization is just the proof of possession for SC addresses.
| Account storage account = accounts[_account]; | ||
|
|
||
| address signer; | ||
| if (keccak256(abi.encodePacked(role)) == keccak256(abi.encodePacked(ValidatorSigner))) { |
There was a problem hiding this comment.
there is a lot of redundancy in these if/elses (switch)
can we abstract?
There was a problem hiding this comment.
TBH I don't see how. We have this switch block in four places but I'm not sure what an extracted function would look like as we're doing different things each time.
| } | ||
|
|
||
| function hasDefaultSigner(address account, string memory role) public view returns (bool) { | ||
| require(isAccount(account)); |
There was a problem hiding this comment.
eth_call reverting isn't super useful imo
There was a problem hiding this comment.
What if the view is used on a non-view function tho?
| * @param _account The address of the account. | ||
| * @param role The role of the signer. | ||
| */ | ||
| function getDefaultSigner(address _account, string memory role) public view returns (address) { |
There was a problem hiding this comment.
diito on separating for gas efficiency
| } | ||
|
|
||
| function hasDefaultSigner(address account, string memory role) public view returns (bool) { | ||
| require(isAccount(account)); |
There was a problem hiding this comment.
What if the view is used on a non-view function tho?
AlexBHarley
requested review from
barbaraliau,
medhakothari and
a team
as code owners
|
I see that the |
yorhodes
force-pushed
the
yorhodes/library-reuse
branch
2 times, most recently
from
9deb46a
to
b7742f1
Compare
This reverts commit e087d13.
…e walletAddress) without being messy
yorhodes
force-pushed
the
alexbharley/cip10
branch
from
e031172
to
6bec2ff
Compare
Description
For more context take a look at CIP10. There's so many commits as there was a lot of back and forth on implementation details.
Other changes
N/A.
Tested
Added an extensive matrix of tests to test this for backwards compatibility.
Related issues
Other changes
Accounts.solis nowInitializableV2.Backwards compatibility
These changes are backwards compatible.