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

Create LedgerPreprocessHook to reformat EIP-712 payloads #1277

Merged
merged 12 commits into from
Sep 13, 2022

Conversation

0a1c
Copy link
Contributor

@0a1c 0a1c commented Aug 17, 2022

Closes: #XXX

Description

This PR implements the Ledger EIP-712 preprocessing required to format EIP-712 transactions for verification, by including the signature information in a Web3Tx extension, setting the signature in the tx body to be blank, and setting the SignMode to be LEGACY_AMINO for Ledger-signed transactions.

This preprocessing will be performed on each transaction submitted via the CLI; however, all non-Ledger interactions should short-circuit.

This must be modified (if not completely removed) when an Evmos Ledger app becomes available, as that would not benefit from this preprocessing.

Tests

Test Coverage:

  • Basic functionality on populated and unpopulated transactions
  • Short-circuit on non-Ledger transactions
  • Error-checking functionality on blank transactions & invalid chain id

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)

@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2022

This pull request introduces 1 alert when merging d897959 into f11bc35 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@0a1c 0a1c force-pushed the develop/eip712/reformat-tx branch from d897959 to 6dea312 Compare August 31, 2022 00:10
@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2022

This pull request introduces 1 alert when merging 6dea312 into c9fe1d1 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@0a1c 0a1c changed the title Create ReformatLedgerTx to reformat EIP-712 payloads Create LedgerPreprocessHook to reformat EIP-712 payloads Aug 31, 2022
@0a1c 0a1c marked this pull request as ready for review August 31, 2022 02:31
@0a1c 0a1c marked this pull request as draft August 31, 2022 02:33
@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #1277 (ba87b4d) into main (14e2f8d) will decrease coverage by 1.74%.
The diff coverage is 67.85%.

❗ Current head ba87b4d differs from pull request most recent head 7b66432. Consider uploading reports for the commit 7b66432 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1277      +/-   ##
==========================================
- Coverage   56.53%   54.78%   -1.75%     
==========================================
  Files         105      107       +2     
  Lines        9609     9984     +375     
==========================================
+ Hits         5432     5470      +38     
- Misses       3912     4243     +331     
- Partials      265      271       +6     
Impacted Files Coverage Δ
ethereum/eip712/preprocess.go 67.85% <67.85%> (ø)
ethereum/eip712/eip712.go 0.00% <0.00%> (ø)

@0a1c 0a1c marked this pull request as ready for review August 31, 2022 02:38
@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2022

This pull request introduces 1 alert when merging ac17f58 into c9fe1d1 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2022

This pull request introduces 1 alert when merging bbcec73 into 45fdcaf - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

Copy link
Contributor

@danburck danburck left a comment

Choose a reason for hiding this comment

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

@austinchandra LGTM, left some comments on how to approve the comments.

ethereum/eip712/preprocess_test.go Outdated Show resolved Hide resolved
ethereum/eip712/preprocess_test.go Show resolved Hide resolved
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK, left a comment about supporting multisigs. Can you add a feature changelog entry?

ethereum/eip712/preprocess.go Show resolved Hide resolved
ethereum/eip712/preprocess.go Outdated Show resolved Hide resolved
@fedekunze fedekunze requested a review from a team as a code owner September 13, 2022 14:55
@fedekunze fedekunze merged commit 723443a into main Sep 13, 2022
@fedekunze fedekunze deleted the develop/eip712/reformat-tx branch September 13, 2022 14:56
danburck added a commit that referenced this pull request Nov 2, 2022
fedekunze pushed a commit that referenced this pull request Nov 2, 2022
* Revert "feat(eip712): Create LedgerPreprocessHook to reformat EIP-712 payloads (#1277)"

This reverts commit 723443a.

* release: upadte changelog
@danburck danburck mentioned this pull request Nov 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants