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

Implement EIP-712 Typed Data #51

Merged
merged 30 commits into from
Dec 3, 2023

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Oct 24, 2023

Summary

  • Clean Apache 2.0 implementation of https://eips.ethereum.org/EIPS/eip-712
    • Initial implementation believed to contain all core features
  • Re-uses the ABI encoding libraries in this repo wherever possible, for consistent encoding/parsing behavior
    • Had to expose a couple of new side-doors to the ABI package to do this - other consumers could use these too
  • Operation compared with ISC License reference implementation in TypeScript:
  • Implements V4 derivation of the behavior, as adopted by Metamask and OpenZeppelin communities
    • Most questions in the spec resolved through behavior of above reference implementation
  • Extends the ethsigner.Wallet interface with ethsigner.WalletTypedData
    • Avoids existing wallet implementations being forced to change
  • Extends the fswallet.Wallet implementation to include SignTypedDataV4
  • Contains some general maintenance on the repo
    • Updates firefly-common
    • Updates to Go 1.20
    • Updates dependencies
  • Provides mapping of ABI types to EIP-712 type arrays
    • Feels like the tooling around EIP-712 requires quite a bit of hand-spinning for the Solidity developer, and I was thinking this could take away at least one potential source of mistakes, and remove a 3rd artifact to maintain (Solidity struct/ABI, Solidity EIP-712 signing construct on-chain, JSON EIP-712 type array off-chain). Maybe even allow alternative APIs to be built that take ABI type definitions to sign directly.

Outstanding conversations

  • EIP-712 original standard includes this comment:

    Note that the v parameter includes the chain id as specified in EIP-155.

    • I did not find evidence that this is adopted. Instead, there is strong guidance to use chainId in the EIP712Domain structure, where it can be verified on-chain using the current block.chainId.
    • As a result, the ethsigner.SignTypedDataV4 code does not call sig.UpdateEIP155(chainID)

@peterbroadhurst
Copy link
Contributor Author

@awrichar - looks like there's a DCO issue on the initial commit. Will work with you to get that resolved.

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e4d4a70) 100.00% compared to head (04de049) 99.90%.
Report is 5 commits behind head on main.

Files Patch % Lines
pkg/rpcbackend/backend.go 57.14% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main      #51      +/-   ##
===========================================
- Coverage   100.00%   99.90%   -0.10%     
===========================================
  Files           35       38       +3     
  Lines         3015     3317     +302     
===========================================
+ Hits          3015     3314     +299     
- Misses           0        2       +2     
- Partials         0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -199,8 +199,16 @@ func (rc *RPCClient) SyncRequest(ctx context.Context, rpcReq *RPCRequest) (rpcRe
}
// JSON/RPC allows errors to be returned with a 200 status code, as well as other status codes
if res.IsError() || rpcRes.Error != nil && rpcRes.Error.Code != 0 {
log.L(ctx).Errorf("RPC[%s] <-- [%d]: %s", rpcTraceID, res.StatusCode(), rpcRes.Message())
err := fmt.Errorf(rpcRes.Message())
rpcMsg := rpcRes.Message()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was required due to resty update

awrichar and others added 12 commits October 24, 2023 11:24
This is very incomplete - just a start based on reading of EIP-712.

Signed-off-by: Andrew Richardson <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Andrew Richardson <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
@awrichar
Copy link
Contributor

FYI: I went back to add signoff on my commit, then replayed all of @peterbroadhurst 's commits on top. Should be unchanged in terms of the actual code changes.

Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

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

Just a quick question about error codes with a possible duplicate, but happy to merge as is

internal/signermsgs/en_error_messges.go Outdated Show resolved Hide resolved
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst peterbroadhurst merged commit 78ea6df into hyperledger:main Dec 3, 2023
2 checks passed
@peterbroadhurst peterbroadhurst deleted the typed_data branch December 3, 2023 21:04
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.

4 participants