Adds signAuthorization method for EIP-7702
#407
Conversation
jeffsmale90
force-pushed
the
feat/sign-authorization
branch
from
5c46e25 to
bb5e268
Compare
jeffsmale90
force-pushed
the
feat/sign-authorization
branch
from
bb5e268 to
b50bc64
Compare
src/sign-authorization.ts
Outdated
| * @property contractAddress - The address of the contract being authorized. | ||
| * @property nonce - The nonce of the signing account (at the time of submission). | ||
| */ | ||
| export type Authorization = [ |
There was a problem hiding this comment.
Minor, could Authorization be or become too generic a term in the context of signing?
Should we call this something like EIP7702Authorization?
Maybe 7702 as the filename?
There was a problem hiding this comment.
Yeah, I think that makes sense - it's already a bit of an overloaded term.
There was a problem hiding this comment.
I named the file sign-eip7702-authorization.ts for a combination of clarity and consistency with the other signing files.
src/sign-authorization.ts
Outdated
| * @param authorization - The authorization object to validate. | ||
| * @throws {Error} If the authorization object or any of its required parameters are missing. | ||
| */ | ||
| function validateAuthorization(authorization: Authorization) { |
There was a problem hiding this comment.
Minor, non-exported functions at the bottom of the file to highlight what is used externally?
src/sign-authorization.ts
Outdated
|
|
||
| const [chainId, contractAddress, nonce] = authorization; | ||
|
|
||
| if (isNullish(chainId)) { |
There was a problem hiding this comment.
Do we definitely want to support / encourage cross-chain authorizations via the 0 chain ID?
Could we also validate it's not zero at this package level?
There was a problem hiding this comment.
I am not sure whether or not we want to support chainId 0 - there's an argument for it, even though it's problematic (nonce likely won't match across all chains).
My preference is to support it at the utility level, and make the decision at the API level.
src/sign-authorization.ts
Outdated
| * @param options.authorization - The authorization data to sign. | ||
| * @returns The '0x'-prefixed hex encoded signature. | ||
| */ | ||
| export function signAuthorization({ |
There was a problem hiding this comment.
Minor, maybe sign7702Authorization and the same format for the other functions?
Just to add more clarity if we get other types of authorization in future?
There was a problem hiding this comment.
I went with EIP7702 in all the places, I think I like that better than just the number.
src/sign-authorization.test.ts
Outdated
| 'hex', | ||
| ); | ||
|
|
||
| const expectedSignature = |
There was a problem hiding this comment.
Minor, capitals and underscores for constants so EXPECTED_SIGNATURE etc?
src/sign-authorization.test.ts
Outdated
|
|
||
| describe('signAuthorization', () => { | ||
| describe('signAuthorization()', () => { | ||
| it('should produce the correct signature', () => { |
There was a problem hiding this comment.
Minor, should is redundant in test names? produces the correct signature for example.
There was a problem hiding this comment.
You are correct - it could be removed, but should is consistent across all of the existing tests 🤷
src/sign-authorization.test.ts
Outdated
| allZeroValues: [0, '0x0000000000000000000000000000000000000000', 0], | ||
| } as { [key: string]: Authorization }; | ||
|
|
||
| for (const [label, authorization] of Object.entries(testCases)) { |
There was a problem hiding this comment.
Minor, could also use a it.each.
…shuffled non-exported members to the bottom of the file, and renamed a few test constants to aid readability. Also used it.each for multiple test cases.
|
@matthewwalsh0 thanks for the review! I made all of the changes you recommended, except for validating chainId !== 0, as I think this should be an API concern, rather than a utility concern. |
EIP-7702 defines a new struct
Authorizationwhich represents authority to set a pointer to a contract address at an EOA - effectively making the EOA perform as a smart contract.This change adds methods to hash and sign authorizations, and to recover signer address.
Although the EIP defines the signed authorization as
[chain_id, address, nonce, y_parity, r, s], the utility method returns the signature as a standardr|s|vsignature for consistency with other signing methods.See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-7702.md for details