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.

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

Apply Kava patches on v0.21.0 #16

Merged
merged 20 commits into from
Mar 30, 2023
Merged

Apply Kava patches on v0.21.0 #16

merged 20 commits into from
Mar 30, 2023

Conversation

drklee3
Copy link
Member

@drklee3 drklee3 commented Feb 28, 2023

No description provided.

return apitypes.TypedDataDomain{
Name: "Kava Cosmos",
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
@@ -18,9 +18,12 @@ message Params {
repeated int64 extra_eips = 4 [(gogoproto.customname) = "ExtraEIPs", (gogoproto.moretags) = "yaml:\"extra_eips\""];
// chain_config defines the EVM chain configuration parameters
ChainConfig chain_config = 5 [(gogoproto.moretags) = "yaml:\"chain_config\"", (gogoproto.nullable) = false];
// list of allowed eip712 msgs and their types
repeated EIP712AllowedMsg eip712_allowed_msgs = 6
Copy link
Member Author

Choose a reason for hiding this comment

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

Field 6 is previously eip712_allowed_msgs on our fork + mainnet, so to be backwards compatible the new upstream allow_unprotected_txs field is updated to field 7.

Breaking proto CI job considers it breaking since upstream has it field 6.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I think we will have to keep the 6 for our field, and ignore the breaking proto OR update it to check against our previous version. Downside here is that other codecs using ethermint proto's from upstream won't be compatible. There are some other options like migrating to our own proto type or changing the namespace, worth a discussion.

Copy link
Member

Choose a reason for hiding this comment

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

Another option could be to bite the bullet on the breaking codec change and separate upstream param's from our custom parameters.

@drklee3 drklee3 marked this pull request as ready for review February 28, 2023 22:44
@@ -217,7 +223,7 @@ func VerifySignature(
return errorsmod.Wrap(errortypes.ErrNoSignatures, "tx doesn't contain any msgs to verify signature")
}

txBytes := legacytx.StdSignBytes(
txBytes := eip712.ConstructUntypedEIP712Data(
Copy link
Member

Choose a reason for hiding this comment

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

I saw a comment that tx timeout was disable because it wasn't passed through, but appears it is included in the sign doc?

@@ -224,19 +223,7 @@ func (pubKey *PubKey) UnmarshalAminoJSON(bz []byte) error {
//
// CONTRACT: The signature should be in [R || S] format.
func (pubKey PubKey) VerifySignature(msg, sig []byte) bool {
return pubKey.verifySignatureECDSA(msg, sig) || pubKey.verifySignatureAsEIP712(msg, sig)
Copy link

Choose a reason for hiding this comment

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

we will probably want to test the new eip712 signing logic. Looks like it should work well with the eip712 data type construction we are doing. Maybe we should just keep this?

@drklee3 drklee3 temporarily deployed to release March 7, 2023 22:04 — with GitHub Actions Inactive
@@ -120,7 +120,6 @@ func (suite *AnteTestSuite) SetupTest() {
encodingConfig := encoding.MakeConfig(app.ModuleBasics)
// We're using TestMsg amino encoding in some tests, so register it here.
encodingConfig.Amino.RegisterConcrete(&testdata.TestMsg{}, "testdata.TestMsg", nil)
eip712.SetEncodingConfig(encodingConfig)
Copy link

Choose a reason for hiding this comment

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

note that we probably want to include the new eip712 additions in ethermint v21, which is that is is needed for.

@drklee3 drklee3 merged commit d9a405f into kava-release-v0.21.x Mar 30, 2023
@drklee3 drklee3 deleted the v0.21.0-kava branch March 30, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants