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.

Sign up for GitHub

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

Jump to bottom

fix: update EIP712 related functions #78

Merged
merged 8 commits into from
Jan 16, 2023
Merged

fix: update EIP712 related functions #78

merged 8 commits into from
Jan 16, 2023

Conversation

pythonberg1997
Copy link
Contributor

@pythonberg1997 pythonberg1997 commented Dec 26, 2022 •
edited
Loading

Description

Adopt a new solution for EIP712 sign mode. Previously, we use amino codec to marshal the msg in txn and there're a lot of problems. Now we adopt proto codec. Besides that, all *Any types in msg will be treated as bytes rather than unpack it.

Rationale

The PR is bit hard to review, please focus the core modification:
image

Example

N/A

Changes

Notable changes:

  • remove bech32 codec from account keeper and grpc query
  • update EIP712 related funcionts
  • migrate all test cases sign mode to EIP712

@pythonberg1997 pythonberg1997 changed the base branch from master to develop December 26, 2022 08:26
@pythonberg1997 pythonberg1997 changed the title fix: remove bech32 codec from account keeper and grpc query fix: update EIP712 related functions Jan 9, 2023
@pythonberg1997
Merge remote-tracking branch 'origin/develop' into wallet_support
deac15d 
# Conflicts:
#	baseapp/block_gas_test.go
#	go.mod
#	x/staking/simulation/operations.go
@unclezoro
Copy link
Collaborator

image
The error message contains account number, I think it is quite difficulty to locate an account based on account number, so please change to account address.

@unclezoro
Copy link
Collaborator

image

Can you remove the else here just in case, we don't expect any other signing mode besides EIP712.

unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
unclezoro
unclezoro reviewed Jan 10, 2023
View reviewed changes
x/auth/tx/eip712.go Outdated Show resolved Hide resolved
@pythonberg1997
Copy link
Contributor Author

pythonberg1997 commented Jan 10, 2023 •
edited
Loading

reference:
https://github.com/evmos/ethermint/blob/57ed355c985d9f3116aba6aabfa2ee0f3f38e966/ethereum/eip712/eip712.go

@pythonberg1997 pythonberg1997 force-pushed the remove_bech32 branch 3 times, most recently from 77b549d to ed21e90 Compare January 12, 2023 11:35
unclezoro
unclezoro reviewed Jan 13, 2023
View reviewed changes
types/address.go
@@ -279,7 +297,7 @@ func (aa AccAddress) Bytes() []byte {

// String implements the Stringer interface.
func (aa AccAddress) String() string {
if aa.Empty() {
if len(aa) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To me, I always expect the String() of AccAddress return a valid format hex, "" is not expected in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

unclezoro
unclezoro reviewed Jan 13, 2023
View reviewed changes
types/address.go
@@ -413,7 +431,7 @@ func (va ValAddress) Bytes() []byte {

// String implements the Stringer interface.
func (va ValAddress) String() string {
if va.Empty() {
if len(va) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

so is ValAddress

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

unclezoro
unclezoro reviewed Jan 13, 2023
View reviewed changes
x/auth/ante/sigverify.go Outdated Show resolved Hide resolved
@unclezoro
Copy link
Collaborator

The rest LGTM.

@unclezoro unclezoro merged commit 274f929 into develop Jan 16, 2023
@yutianwu yutianwu mentioned this pull request Jan 18, 2023
Merged
@pythonberg1997 pythonberg1997 deleted the remove_bech32 branch January 31, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unclezoro unclezoro unclezoro approved these changes

Assignees
No one assigned
Labels
C:CLI C:Simulations C:x/auth C:x/authz C:x/bank C:x/distribution C:x/evidence C:x/feegrant C:x/genutil C:x/gov C:x/group C:x/nft C:x/slashing C:x/staking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pythonberg1997 @unclezoro