feat: add EIP712 support for multiple messages and schemas #1390

0a1c · 2023-02-13T19:58:43Z

Description

Reopens the original PR from Ethermint: evmos/ethermint#1593

In addition to the changes listed in the original PR:

Refactor eip712.go into eip712/types.go, eip712/message.go, eip712/domain.go, and eip712/eip712.go for better modularity and code-clarity.
Remove feePayer as it's no longer necessary.

Refactor the EIP-712 Types and TypedData generation algorithm to provide more flexibility in type-generation and signing.

Reference

https://www.notion.so/evmos/EIP-712-Refactor-Different-Types-of-Messages-1ae3ddf5bc66428997722634f091d346

Motivation

The current EIP-712 TypedData generation algorithm groups all messages into one array. Due to the way EIP-712's typing system is defined, all items in an array must share the same schema. This means that Cosmos payloads with different types of messages, or messages with the same type of schema, cannot be signed using the current algorithm.

When it comes to users and developers, this issue limits Ledger and Metamask users in terms of their overall capabilities, since they cannot sign certain payloads—the same applies to developers aiming to provide certain functionality to users.

Implementation

Fundamentally, this refactor changes the EIP-712 algorithm from using the sdk.Msg Go object to generate types to using the payload message in JSON to generate types.

In the process, we flatten the JSON payload messages, converting them from an array of JSON objects to numbered field: value pairs. This allows us to define a custom schema for each message when necessary, to support multiple types of messages.

This change also fixes a bug with omitted optional values—we currently check if a given field has value of zero, and exclude it if so, which has the unintended side-effect of removing all zero values that are present. When we use the JSON payload to generate types instead, we automatically exclude values that are missing.

Outcomes

Support messages of different types & schemas
The current implementation groups all payload messages into an array when building the EIP-712 object. Since EIP-712 arrays are strongly typed, this has the side-effect of enforcing all messages to share a single schema. With this change, we can begin to support different messages and different schemas within a single transaction.

Fix bug with omitted values
The current EIP-712 implementation omits values from type generation if they are equal to zero, in order to filter omitted values in the payload when building the types. This creates false negatives, in which values that are included but equal to zero are also filtered, such as false boolean values or numerical values equal to zero.

Legacy Support

The previous implementation is kept in the codebase as eip712_legacy.go and encoding_legacy.go, in order to supports backwards-compatibility with existing products and integrations. This provides Evmos developers a chance to migrate to the new implementation (or a new version of EvmosJS) before we completely remove support for the legacy encoding.

Tests

Fuzz tests for flattening (ethereum/eip712/eip712_fuzzer_test.go)
AnteTests for all messages and new outcomes (app/ante_test.go)
EIP712 error handling and unit tests (ethereum/eip712/eip712_test.go)

ethereum/eip712/eip712_legacy.go

into austinchandra/eip712-m1

ethereum/eip712/eip712_legacy.go

codecov · 2023-02-14T00:23:30Z

Codecov Report

Merging #1390 (6bd0e30) into main (77eac32) will increase coverage by 0.38%.
The diff coverage is 75.41%.

❗ Current head 6bd0e30 differs from pull request most recent head 8bc9210. Consider uploading reports for the commit 8bc9210 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1390      +/-   ##
==========================================
+ Coverage   72.31%   72.70%   +0.38%     
==========================================
  Files         260      265       +5     
  Lines       17732    18206     +474     
==========================================
+ Hits        12823    13236     +413     
- Misses       4337     4380      +43     
- Partials      572      590      +18

Impacted Files	Coverage Δ
app/ante/cosmos/eip712.go	`5.20% <0.00%> (ø)`
crypto/ethsecp256k1/ethsecp256k1.go	`65.71% <0.00%> (-4.70%)`	⬇️
ethereum/eip712/encoding.go	`78.00% <ø> (+1.31%)`	⬆️
ethereum/eip712/eip712_legacy.go	`60.32% <60.32%> (ø)`
ethereum/eip712/encoding_legacy.go	`77.04% <77.04%> (ø)`
ethereum/eip712/message.go	`81.92% <81.92%> (ø)`
ethereum/eip712/types.go	`92.34% <92.34%> (ø)`
ethereum/eip712/domain.go	`100.00% <100.00%> (ø)`
ethereum/eip712/eip712.go	`100.00% <100.00%> (+39.87%)`	⬆️

ethereum/eip712/eip712_fuzzer_test.go

crypto/ethsecp256k1/ethsecp256k1.go

app/ante/evm/utils_test.go

ethereum/eip712/encoding_legacy.go

GAtom22

Great work @austinchandra !! Left a few comments and a question

app/ante/evm/ante_test.go

ethereum/eip712/eip712_test.go

ethereum/eip712/encoding.go

linear · 2023-02-20T14:39:45Z

ENG-1490 Re-open EIP-712 support for multiple messages on Evmos

Was originally open on Ethermint, reopen on Evmos.

ethereum/eip712/types.go

+	for k := range jsonMap {
+		keys[i] = k
+		i++
+	}


GAtom22

LGTM! Great work @austinchandra !! There are some lint errors related to constants using all capital letters

fedekunze

ACK with minor comments. Great work @austinchandra

ethereum/eip712/domain.go

ethereum/eip712/types.go

ethereum/eip712/message.go

ethereum/eip712/eip712_legacy.go

ethereum/eip712/eip712_test.go

ethereum/eip712/domain.go

ethereum/eip712/eip712_legacy.go

+	"encoding/json"
+	"fmt"
+	"math/big"
+	"reflect" // #nosec G702 for sensitive import


ethereum/eip712/domain.go

+	domain := apitypes.TypedDataDomain{
+		Name:              "Cosmos Web3",
+		Version:           "1.0.0",
+		ChainId:           math.NewHexOrDecimal256(int64(chainID)), // #nosec G701


feat: add EIP712 support for multiple messages and schemas

59432f2

0a1c requested a review from a team as a code owner February 13, 2023 19:58

0a1c requested review from ramacarlucho, GAtom22 and facs95 and removed request for a team February 13, 2023 19:58

Merge branch 'main' into austinchandra/eip712-m1

87b12e1

github-advanced-security bot found potential problems Feb 13, 2023

View reviewed changes

ethereum/eip712/eip712_legacy.go Fixed Show fixed Hide fixed

ethereum/eip712/eip712_legacy.go Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Feb 13, 2023

View reviewed changes

ethereum/eip712/eip712_legacy.go Fixed Show fixed Hide fixed

facs95 marked this pull request as draft February 13, 2023 21:53

0a1c added 3 commits February 13, 2023 14:23

Merge branch 'main' into austinchandra/eip712-m1

46aca8a

Fix lint issues and codescans, add documentation

772e79d

Merge branch 'austinchandra/eip712-m1' of https://github.com/evmos/evmos

c1dec8b

into austinchandra/eip712-m1

0a1c commented Feb 13, 2023

View reviewed changes

ethereum/eip712/eip712_legacy.go Outdated Show resolved Hide resolved

0a1c marked this pull request as ready for review February 13, 2023 22:43

0a1c enabled auto-merge (squash) February 13, 2023 22:45

github-advanced-security bot found potential problems Feb 13, 2023

View reviewed changes

ethereum/eip712/eip712_legacy.go Fixed Show fixed Hide fixed

fedekunze reviewed Feb 14, 2023

View reviewed changes

0a1c added 3 commits February 14, 2023 09:45

Use LegacyEIP712 verification only if necessary

22d5f8a

Merge branch 'main' into austinchandra/eip712-m1

340019e

Merge main and address conflicts

8bd2a24

GAtom22 reviewed Feb 17, 2023

View reviewed changes

fedekunze marked this pull request as draft February 21, 2023 11:17

auto-merge was automatically disabled February 21, 2023 11:17
Pull request was converted to draft

0a1c added 4 commits February 21, 2023 16:33

Merge branch 'main' into austinchandra/eip712-m1

f78848d

Fix set of revision comments

f57bbd5

Clean up EIP712 package tests

9fb370f

Further refactor EIP712 fuzzer test for clarity

6f6b135

0a1c added 7 commits February 22, 2023 13:34

Improve test coverage and clarity

22d5186

Refactor eip712/messages for clarity

b96fcfd

Improve eip712/types code clarity

8085ee3

Further refactor eip712/types for clarity

eb168ef

Improve eip712/types function and variable naming

c801995

Merge branch 'main' into austinchandra/eip712-m1

8e15873

Refactor tests to account for legacy TypedData/Extension

fc11de4

github-advanced-security bot found potential problems Feb 23, 2023

View reviewed changes

ethereum/eip712/types.go Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Feb 23, 2023

View reviewed changes

ethereum/eip712/types.go

Comment on lines +248 to +251

for k := range jsonMap {

keys[i] = k

i++

}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism

0a1c added 2 commits February 22, 2023 20:09

Improve test code quality

95d4b76

Add changelog entry

bb9f7d4

0a1c marked this pull request as ready for review February 23, 2023 04:11

0a1c requested review from fedekunze and GAtom22 and removed request for ramacarlucho February 23, 2023 04:11

Improve test coverage and hide gosec map error

adfd495

GAtom22 approved these changes Feb 23, 2023

View reviewed changes

fedekunze approved these changes Feb 23, 2023

View reviewed changes

0a1c added 4 commits February 24, 2023 20:38

Merge branch 'main' into austinchandra/eip712-m1

6959e9b

Remove chainID overflow safety check

2ebf382

Refactor all caps to camelCase

b9faf9a

Remove error in eip712/domain

6bd0e30

0a1c enabled auto-merge (squash) February 25, 2023 05:13

github-advanced-security bot found potential problems Feb 25, 2023

View reviewed changes

ethereum/eip712/domain.go Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Feb 25, 2023

View reviewed changes

ethereum/eip712/eip712_legacy.go

"encoding/json"

"fmt"

"math/big"

"reflect" // #nosec G702 for sensitive import

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

Add nosec rule for possible int overflow

8bc9210

github-advanced-security bot found potential problems Feb 25, 2023

View reviewed changes

0a1c merged commit a17e0e2 into main Feb 25, 2023

0a1c deleted the austinchandra/eip712-m1 branch February 25, 2023 05:44

0a1c mentioned this pull request Apr 10, 2023

Migrate updated EIP-712 algorithm from Evmos evmos/ethermint#1746

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add EIP712 support for multiple messages and schemas #1390

feat: add EIP712 support for multiple messages and schemas #1390

0a1c commented Feb 13, 2023 •

edited by fedekunze

codecov bot commented Feb 14, 2023 •

edited

GAtom22 left a comment

linear bot commented Feb 20, 2023

GAtom22 left a comment

fedekunze left a comment

feat: add EIP712 support for multiple messages and schemas #1390

feat: add EIP712 support for multiple messages and schemas #1390

Conversation

0a1c commented Feb 13, 2023 • edited by fedekunze

Description

Reference

Motivation

Implementation

Outcomes

Legacy Support

Tests

codecov bot commented Feb 14, 2023 • edited

Codecov Report

GAtom22 left a comment

Choose a reason for hiding this comment

linear bot commented Feb 20, 2023

GAtom22 left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

0a1c commented Feb 13, 2023 •

edited by fedekunze

codecov bot commented Feb 14, 2023 •

edited