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

Refactor to omit empty optionals from EIP-712 type generation #1459

Merged
merged 7 commits into from
Nov 17, 2022
Merged

Refactor to omit empty optionals from EIP-712 type generation #1459

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8990d38
Refactor to omit empty values from type creation; add test for v1.vote
0a1c Nov 14, 2022
6648c08
Add test for createValidator with optional fields left blank
0a1c Nov 14, 2022
4a0e43e
Add changelog entry
0a1c Nov 14, 2022
a841e97
Update changelog entry
0a1c Nov 15, 2022
7ffeb3e
Merge branch 'main' into austin/ENG-975
facs95 Nov 16, 2022
b2727c2
Merge branch 'main' into austin/ENG-975
facs95 Nov 16, 2022
aedde92
Merge branch 'main' into austin/ENG-975
facs95 Nov 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (feemarket) [#1194](https://github.com/evmos/ethermint/pull/1194) Apply feemarket to native cosmos tx.
* (eth) [#1346](https://github.com/evmos/ethermint/pull/1346) Added support for `sdk.Dec` and `ed25519` type on eip712.
* (ante) [1460](https://github.com/evmos/ethermint/pull/1460) Add KV Gas config on ethereum Txs.
* (eth) [#1459](https://github.com/evmos/ethermint/pull/1459) Added support for messages with optional types omitted on eip712.

### API Breaking

Expand Down
22 changes: 22 additions & 0 deletions app/ante/ante_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,17 @@ func (suite AnteTestSuite) TestAnteHandler() {
return txBuilder.GetTx()
}, false, false, true,
},
{
"success- DeliverTx EIP712 create validator (with blank fields)",
func() sdk.Tx {
from := acc.GetAddress()
coinAmount := sdk.NewCoin(evmtypes.DefaultEVMDenom, sdk.NewInt(20))
amount := sdk.NewCoins(coinAmount)
gas := uint64(200000)
txBuilder := suite.CreateTestEIP712MsgCreateValidator2(from, privKey, "ethermint_9000-1", gas, amount)
return txBuilder.GetTx()
}, false, false, true,
},
{
"success- DeliverTx EIP712 MsgSubmitProposal",
func() sdk.Tx {
Expand Down Expand Up @@ -413,6 +424,17 @@ func (suite AnteTestSuite) TestAnteHandler() {
return txBuilder.GetTx()
}, false, false, true,
},
{
"success- DeliverTx EIP712 MsgVoteV1",
func() sdk.Tx {
from := acc.GetAddress()
coinAmount := sdk.NewCoin(evmtypes.DefaultEVMDenom, sdk.NewInt(20))
amount := sdk.NewCoins(coinAmount)
gas := uint64(200000)
txBuilder := suite.CreateTestEIP712MsgVoteV1(from, privKey, "ethermint_9000-1", gas, amount)
return txBuilder.GetTx()
}, false, false, true,
},
{
"fails - DeliverTx EIP712 signed Cosmos Tx with wrong Chain ID",
func() sdk.Tx {
Expand Down
24 changes: 23 additions & 1 deletion app/ante/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
evtypes "github.com/cosmos/cosmos-sdk/x/evidence/types"
"github.com/cosmos/cosmos-sdk/x/feegrant"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
types5 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"
"github.com/evmos/ethermint/app"
ante "github.com/evmos/ethermint/app/ante"
Expand Down Expand Up @@ -288,7 +289,6 @@ func (suite *AnteTestSuite) CreateTestEIP712MsgCreateValidator(from sdk.AccAddre
valAddr,
privEd.PubKey(),
sdk.NewCoin(evmtypes.DefaultEVMDenom, sdk.NewInt(20)),
// TODO: can this values be empty strings?
types3.NewDescription("moniker", "indentity", "website", "security_contract", "details"),
types3.NewCommissionRates(sdk.OneDec(), sdk.OneDec(), sdk.OneDec()),
sdk.OneInt(),
Expand All @@ -297,6 +297,23 @@ func (suite *AnteTestSuite) CreateTestEIP712MsgCreateValidator(from sdk.AccAddre
return suite.CreateTestEIP712CosmosTxBuilder(from, priv, chainId, gas, gasAmount, msgCreate)
}

func (suite *AnteTestSuite) CreateTestEIP712MsgCreateValidator2(from sdk.AccAddress, priv cryptotypes.PrivKey, chainId string, gas uint64, gasAmount sdk.Coins) client.TxBuilder {
// Build MsgCreateValidator
valAddr := sdk.ValAddress(from.Bytes())
privEd := ed25519.GenPrivKey()
msgCreate, err := types3.NewMsgCreateValidator(
valAddr,
privEd.PubKey(),
sdk.NewCoin(evmtypes.DefaultEVMDenom, sdk.NewInt(20)),
// Ensure optional fields can be left blank
types3.NewDescription("moniker", "indentity", "", "", ""),
types3.NewCommissionRates(sdk.OneDec(), sdk.OneDec(), sdk.OneDec()),
sdk.OneInt(),
)
suite.Require().NoError(err)
return suite.CreateTestEIP712CosmosTxBuilder(from, priv, chainId, gas, gasAmount, msgCreate)
}

func (suite *AnteTestSuite) CreateTestEIP712SubmitProposal(from sdk.AccAddress, priv cryptotypes.PrivKey, chainId string, gas uint64, gasAmount sdk.Coins, deposit sdk.Coins) client.TxBuilder {
proposal, ok := types5.ContentFromProposalType("My proposal", "My description", types5.ProposalTypeText)
suite.Require().True(ok)
Expand Down Expand Up @@ -343,6 +360,11 @@ func (suite *AnteTestSuite) CreateTestEIP712MsgSubmitEvidence(from sdk.AccAddres
return suite.CreateTestEIP712CosmosTxBuilder(from, priv, chainId, gas, gasAmount, msgEvidence)
}

func (suite *AnteTestSuite) CreateTestEIP712MsgVoteV1(from sdk.AccAddress, priv cryptotypes.PrivKey, chainId string, gas uint64, gasAmount sdk.Coins) client.TxBuilder {
msgVote := govtypes.NewMsgVote(from, 1, govtypes.VoteOption_VOTE_OPTION_YES, "")
return suite.CreateTestEIP712CosmosTxBuilder(from, priv, chainId, gas, gasAmount, msgVote)
}

// StdSignBytes returns the bytes to sign for a transaction.
func StdSignBytes(cdc *codec.LegacyAmino, chainID string, accnum uint64, sequence uint64, timeout uint64, fee legacytx.StdFee, msgs []sdk.Msg, memo string, tip *txtypes.Tip) []byte {
msgsBytes := make([]json.RawMessage, 0, len(msgs))
Expand Down
4 changes: 2 additions & 2 deletions ethereum/eip712/eip712.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,8 @@ func traverseFields(
// then continue as normal
}

// If its a nil pointer, do not include in types
if fieldType.Kind() == reflect.Ptr && field.IsNil() {
// If field is an empty value, do not include in types, since it will not be present in the object
if field.IsZero() {
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

We pushed a “fix” earlier that addresses this—it omits empty values from type generation. It’s a fix in quotes because it causes new issues—it rejects empty values that are not omitted as well.

@austinchandra perhaps using IsOmittableField(field) (see below) instead would resolve the problem you mentioned?

// IsOmittableField reports whether v is an omittable value for its type.
// It panics if the argument is invalid.
func IsOmittableField(v reflect.Value) bool {
	switch v.Kind() {
	case reflect.Array:
		for i := 0; i < v.Len(); i++ {
			if !v.Index(i).IsZero() {
				return false
			}
		}
		return true
	case reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice:
		return v.IsNil()
	case reflect.String:
		return v.Len() == 0
	case reflect.Struct:
		for i := 0; i < v.NumField(); i++ {
			if !v.Field(i).IsZero() {
				return false
			}
		}
		return true
	default:
		return false
	}
}

Copy link

@albertchon albertchon Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I see you went with an alternate approach

ethermint/ethereum/eip712/eip712.go

Lines 338 to 340 in 16daa44

if !field.Exists() {
continue
}

Does this code work? Saw the PR was closed

continue
}

Expand Down