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

fix: eip712 support for pubkey any messages #1076

Closed
wants to merge 6 commits into from
Closed

Conversation

hanchon
Copy link
Contributor

@hanchon hanchon commented May 4, 2022

Closes: #XXX

Description

Most messages with EIP712 are working fine, but the code is not working if some values that are not the cosmos transaction itself, are contained inside a proto.Any object.

The issue is that when we create read the transactions values, they are parsed using the JSON lib, for example, for public keys it returns the base64 representation of the pubkey as a string.

When the lib generates the types for the EIP712 message, it uses codec.UnpackAny if the message is contained inside a proto.Any object it will return {type:string, value:...} instead of the expected string

For now the only value that I found that is sent as a proto.Any message is the pubkey, but I need to review the rest of the cosmos-sdk messages


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)

@zmanian
Copy link

zmanian commented May 4, 2022

This PR would close
evmos/evmos#545

@fedekunze
Copy link
Contributor

@zmanian we also would need to remove the single signature check as @tony-iqlusion mentioned

@zmanian
Copy link

zmanian commented May 4, 2022

Iqlusion would be willing to join the validator set with a single sig ledger backed key.

@zmanian
Copy link

zmanian commented May 4, 2022

It's a partial fix for the issue.

@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #1076 (03cc04b) into main (d559893) will increase coverage by 0.09%.
The diff coverage is n/a.

❗ Current head 03cc04b differs from pull request most recent head f7ea579. Consider uploading reports for the commit f7ea579 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1076      +/-   ##
==========================================
+ Coverage   59.39%   59.49%   +0.09%     
==========================================
  Files          85       84       -1     
  Lines        6992     6940      -52     
==========================================
- Hits         4153     4129      -24     
+ Misses       2622     2598      -24     
+ Partials      217      213       -4     
Impacted Files Coverage Δ
rpc/types/utils.go 0.00% <0.00%> (ø)
x/evm/client/cli/query.go 0.00% <0.00%> (ø)
x/evm/migrations/v2/migration.go
x/evm/keeper/state_transition.go 75.54% <0.00%> (+0.16%) ⬆️
x/evm/module.go 53.73% <0.00%> (+0.87%) ⬆️
x/evm/types/params.go 100.00% <0.00%> (+6.52%) ⬆️
x/evm/keeper/migrations.go 100.00% <0.00%> (+33.33%) ⬆️

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days-before-close if no further activity occurs.

@fedekunze fedekunze added pinned Pinned issues that won't be closed by stalebot and removed Status: Stale labels Jul 11, 2022
@facs95 facs95 linked an issue Aug 12, 2022 that may be closed by this pull request
@facs95
Copy link
Contributor

facs95 commented Aug 19, 2022

Replaced by this #1283

@facs95 facs95 closed this Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pinned Pinned issues that won't be closed by stalebot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eip712 doesn't support any messages
5 participants