Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Migrate updated EIP-712 algorithm from Evmos #1746

Merged
merged 5 commits into from
Apr 11, 2023
Merged

Migrate updated EIP-712 algorithm from Evmos #1746

merged 5 commits into from
Apr 11, 2023

Conversation

0a1c
Copy link
Contributor

@0a1c 0a1c commented Apr 10, 2023

Closes: #XXX

Description

Migrate from evmos/evmos#1390.

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@github-actions github-actions bot added the C:Crypto crypto package label Apr 10, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 10, 2023
View reviewed changes
ethereum/eip712/eip712_legacy.go
domain := apitypes.TypedDataDomain{
Name: "Cosmos Web3",
Version: "1.0.0",
ChainId: math.NewHexOrDecimal256(int64(chainID)),

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion

Potential integer overflow by integer type conversion
ethereum/eip712/domain.go
domain := apitypes.TypedDataDomain{
Name: "Cosmos Web3",
Version: "1.0.0",
ChainId: math.NewHexOrDecimal256(int64(chainID)), // #nosec G701

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion

Potential integer overflow by integer type conversion
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ignore not working?

ethereum/eip712/types.go
return
}

*err = fmt.Errorf("%v", r)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack.

PANIC!
@codecov
Copy link

codecov bot commented Apr 10, 2023 •
edited
Loading

Codecov Report

Merging #1746 (4270a90) into main (dfd99e0) will increase coverage by 0.84%.
The diff coverage is 75.59%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1746      +/-   ##
==========================================
+ Coverage   68.05%   68.89%   +0.84%     
==========================================
  Files         113      117       +4     
  Lines       10263    10681     +418     
==========================================
+ Hits         6984     7359     +375     
- Misses       2869     2900      +31     
- Partials      410      422      +12
Impacted Files Coverage Δ
crypto/ethsecp256k1/ethsecp256k1.go 65.71% <0.00%> (-4.70%) ⬇️
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%> (ø)
app/ante/eip712.go 58.46% <100.00%> (ø)
ethereum/eip712/domain.go 100.00% <100.00%> (ø)
ethereum/eip712/eip712.go 100.00% <100.00%> (+39.87%) ⬆️
ethereum/eip712/encoding.go 78.00% <100.00%> (+1.31%) ⬆️

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 10, 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
ethereum/eip712/types.go
Comment on lines +249 to +252
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 0a1c marked this pull request as ready for review April 10, 2023 22:10
@0a1c 0a1c requested a review from a team as a code owner April 10, 2023 22:10
@0a1c 0a1c requested review from facs95 and MalteHerrmann and removed request for a team April 10, 2023 22:10
facs95
facs95 reviewed Apr 11, 2023
View reviewed changes
Copy link
Contributor

@facs95 facs95 left a comment

Choose a reason for hiding this comment

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

Integration tests are failing

ethereum/eip712/domain.go
domain := apitypes.TypedDataDomain{
Name: "Cosmos Web3",
Version: "1.0.0",
ChainId: math.NewHexOrDecimal256(int64(chainID)), // #nosec G701
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ignore not working?

facs95
facs95 suggested changes Apr 11, 2023
View reviewed changes
app/ante/utils_test.go Outdated Show resolved Hide resolved
@0a1c 0a1c enabled auto-merge (squash) April 11, 2023 16:03
@facs95 facs95 disabled auto-merge April 11, 2023 16:03
@facs95 facs95 merged commit 382a6ae into main Apr 11, 2023
@facs95 facs95 deleted the austinchandra/migrate-eip712-feat branch April 11, 2023 16:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@facs95 facs95 facs95 approved these changes

@MalteHerrmann MalteHerrmann Awaiting requested review from MalteHerrmann MalteHerrmann is a code owner automatically assigned from evmos/core-engineering

Assignees
No one assigned
Labels
C:Crypto crypto package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@0a1c @facs95