fix/EIP712_wrapper_v1_for_master #277
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[lintersdb] The name "gas" is deprecated. The linter has been renamed to: gosec." WalkthroughThe pull request introduces significant changes to the EIP-712 transaction handling in the Cosmos SDK. The modifications primarily focus on enhancing message marshaling, codec handling, and type conversion for EIP-712 transactions. A new interface Changes
Sequence DiagramsequenceDiagram
participant Client
participant EIP712Wrapper
participant Codec
participant MessageMarshaler
Client->>EIP712Wrapper: Prepare Transaction
EIP712Wrapper->>Codec: Use AminoProtoCodecMarshaler
Codec->>MessageMarshaler: Marshal Messages to JSON
MessageMarshaler-->>Codec: Return Marshaled Messages
EIP712Wrapper->>EIP712Wrapper: Process Transaction Data
EIP712Wrapper-->>Client: Return Wrapped Transaction
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
eip712_cosmos.go (1)
73-97: Simplify Message Marshaling LogicThe current implementation separately marshals each message into Amino JSON and then reconstructs the
msgsfield. Consider simplifying this by directly marshaling the entiremsgsslice, if possible, to improve efficiency and reduce complexity.Apply this diff to streamline the message marshaling:
msgsData := make(map[string]interface{}) -bzMsgs, err := json.Marshal(map[string][]json.RawMessage{ - "msgs": msgsJsons, -}) +bzMsgs, err := cdc.MarshalAminoJSON(struct { + Msgs []cosmtypes.Msg `json:"msgs"` +}{ + Msgs: msgs, +}) if err != nil { return typeddata.TypedData{}, errors.Wrap(err, "marshal msgs JSON") } -if err := json.Unmarshal(bzMsgs, &msgsData); err != nil { - err = errors.Wrap(err, "failed to unmarshal msgs from proto-compatible amino JSON") - return typeddata.TypedData{}, err -} -txData["msgs"] = msgsData["msgs"] +if err := json.Unmarshal(bzMsgs, &txData); err != nil { + err = errors.Wrap(err, "failed to unmarshal msgs into txData") + return typeddata.TypedData{}, err +}typeddata/typed_data.go (3)
863-875: Clarify Chain ID Mapping LogicThe chain ID mapping replaces certain chain IDs with hardcoded values (e.g.,
777and888mapped to11155111, and others to1). This could lead to confusion or errors if new chain IDs are introduced. Consider making this mapping configurable or documenting the rationale behind these choices.
969-983: Avoid Using Named Return Values for Error RecoveryUsing named return values in conjunction with deferred functions for error handling (as in
doRecover) can be prone to errors and make the code harder to maintain. Consider refactoring to pass the error explicitly or use other error handling mechanisms.
1283-1313: Simplify Chain ID Parsing LogicThe
ParseCosmosChainIDfunction uses a complex regular expression to parse the chain ID. Simplify the regex or consider alternative parsing methods to improve readability and maintainability.eip712_cosmos_test.go (3)
46-56: Redundant Wrapper Function in TestThe
wrapperfunction defined in the test simply callsWrapTxToEIP712. Since it doesn't add any additional logic, consider callingWrapTxToEIP712directly to simplify the test code.Apply this diff to remove the redundant wrapper:
-// Create wrapper function -wrapper := func( - cdc *codec.ProtoCodec, - chainID uint64, - signerData *authsigning.SignerData, - timeoutHeight uint64, - memo string, - feeInfo legacytx.StdFee, - msgs []cosmtypes.Msg, - feeDelegation *FeeDelegationOptions, -) (typeddata.TypedData, error) { - return WrapTxToEIP712(cdc, chainID, signerData, timeoutHeight, memo, feeInfo, msgs, feeDelegation) -} - // Test wrapper execution -typedData, err := wrapper( +typedData, err := WrapTxToEIP712( protoCodec, uint64(testReq.ChainID), &authsigning.SignerData{ ChainID: "injective-777",
283-302: ExportprepareEip712Payloadand Related TypesThe types
prepareEip712Payload,cosmosTxFee,cosmosCoin, anduint64AsStringare defined in the test file but may be useful in other tests or packages. Consider exporting these types and moving them to a shared location if they are generally applicable.
331-340: Add Error Handling for JSON UnmarshalIn the
UnmarshalJSONmethod foruint64AsString, consider handling the case where the incoming JSON value is a number and not a string. This will make the code more robust when dealing with different JSON formats.Apply this diff to handle numeric values:
func (u *uint64AsString) UnmarshalJSON(data []byte) error { - var s string - if err := json.Unmarshal(data, &s); err != nil { - return err - } - v, err := strconv.ParseUint(s, 10, 64) + var v uint64 + if err := json.Unmarshal(data, &v); err != nil { + var s string + if err := json.Unmarshal(data, &s); err != nil { + return err + } + var err error + v, err = strconv.ParseUint(s, 10, 64) + if err != nil { + return err + } + } *u = uint64AsString(v) return nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eip712_cosmos.go(7 hunks)eip712_cosmos_test.go(1 hunks)typeddata/typed_data.go(3 hunks)
🔇 Additional comments (6)
eip712_cosmos.go (5)
1-1: Package Change ConfirmationThe change from
package antetopackage sdkis acceptable if it aligns with the new module structure and promotes better organization.
27-33: Introduction ofAminoProtoCodecMarshalerInterfaceThe new
AminoProtoCodecMarshalerinterface extendscodec.Codecwith aMarshalAminoJSONmethod. This addition enhances flexibility by allowing the codec to marshal messages into Amino JSON format, which is necessary for EIP-712 transaction processing.
Line range hint
49-64: Verify Passing Empty Messages toStdSignBytesIn line 64, the
msgsparameter passed tolegacytx.StdSignBytesis replaced with an empty slice[]cosmtypes.Msg{}. Please verify that passing an empty message slice does not adversely affect the computation of sign bytes or any downstream processes that rely on the messages. It's essential to ensure that all necessary message data is correctly included in the transaction.
Line range hint
111-118: EnsurefeePayerField Alignment in EIP-712 TypesWhen adding the
feePayerto thefeestructure, make sure that theFeetype in themsgTypesmap reflects this change. The current implementation updatesmsgTypes["Fee"], but if other parts of the code rely on the originalFeestructure, this might cause inconsistencies.
Line range hint
525-581: Consistency inWrapTxToEIP712V2FunctionThe
WrapTxToEIP712V2function mirrors changes made inWrapTxToEIP712. Ensure that both functions maintain consistent behavior, especially concerning fee delegation and message handling. Verify that the domain information and the typed data structure conform to the expected EIP-712 standards.typeddata/typed_data.go (1)
859-861: Avoid Settingtimeout_heightto Zero ImplicitlySetting
timeout_heightto "0" when it isnilmight lead to unintended behavior. Ensure that this default value aligns with the expected transaction semantics and consider whether a different default or an explicit error would be more appropriate.
|
|
||
| var ( | ||
| LegacyAminoCodec *codec.LegacyAmino | ||
| ProtoCodec *codec.ProtoCodec | ||
| ) | ||
|
|
||
| func SetCodec(amino *codec.LegacyAmino, proto *codec.ProtoCodec) { | ||
| LegacyAminoCodec, ProtoCodec = amino, proto | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Thread Safety Consideration for Global Codec Variables
The global variables LegacyAminoCodec and ProtoCodec are set using SetCodec, which might lead to concurrency issues in a multi-threaded environment. Consider avoiding global state or using synchronization mechanisms to prevent race conditions.
| // parse txData to StdSignDoc in order to parse the msg for ExtractMsgTypes | ||
| var signDoc legacytx.StdSignDoc | ||
| if err := LegacyAminoCodec.UnmarshalJSON(signDocBytes, &signDoc); err != nil { | ||
| return TypedData{}, err | ||
| } | ||
|
|
||
| var msg cosmtypes.Msg | ||
| if err := LegacyAminoCodec.UnmarshalJSON(signDoc.Msgs[0], &msg); err != nil { | ||
| return TypedData{}, errors.Wrap(err, "failed to unmarshal msg") | ||
| } | ||
|
|
||
| // For some reason this type cast does not work during UnmarshalJSON above | ||
| // If the underlying msg does implement the UnpackInterfacesMessage interface (MsgGrant, MsgExec...), | ||
| // we explicitly call the method here to ensure potential Any fields within the message are correctly parsed | ||
| if unpacker, ok := msg.(codectypes.UnpackInterfacesMessage); ok { | ||
| if err := unpacker.UnpackInterfaces(codectypes.AminoJSONUnpacker{Cdc: LegacyAminoCodec.Amino}); err != nil { | ||
| return TypedData{}, errors.Wrap(err, "failed to unpack msg") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle Multiple Messages in GetEIP712TypedDataForMsg
The current implementation only unmarshals and processes the first message in signDoc.Msgs. If multiple messages are included, the additional messages are ignored. Update the function to handle all messages appropriately.
| func walkFields(cdc codec.ProtoCodecMarshaler, typeMap Types, rootType string, in interface{}) (err error) { | ||
| defer doRecover(&err) | ||
|
|
||
| t := reflect.TypeOf(in) | ||
| v := reflect.ValueOf(in) | ||
|
|
||
| for { | ||
| if t.Kind() == reflect.Ptr || t.Kind() == reflect.Interface { | ||
| t = t.Elem() | ||
| v = v.Elem() | ||
| continue | ||
| } | ||
| break | ||
| } | ||
|
|
||
| err = traverseFields(cdc, typeMap, rootType, typeDefPrefix, t, v) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential Panic in walkFields Function
The walkFields function depends on doRecover to handle panics. Relying on panic and recover for control flow is generally discouraged. Refactor the code to handle errors without panicking to improve reliability and readability.
| // ComputeTypedDataAndHash computes the typed data and its keccak hash for signing | ||
| func ComputeTypedDataAndHash(typedData TypedData) (hash, data []byte, err error) { | ||
| domainSeparator, err := typedData.HashStruct("EIP712Domain", typedData.Domain.Map()) | ||
| if err != nil { | ||
| err = errors.Wrap(err, "failed to pack and hash typedData EIP712Domain") | ||
| return nil, err | ||
| return nil, nil, errors.Wrap(err, "failed to pack and hash typedData EIP712Domain") | ||
| } | ||
|
|
||
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) | ||
| if err != nil { | ||
| err = errors.Wrap(err, "failed to pack and hash typedData primary type") | ||
| return nil, err | ||
| return nil, nil, errors.Wrap(err, "failed to pack and hash typedData EIP712Domain") | ||
| } | ||
|
|
||
| rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash))) | ||
| return crypto.Keccak256(rawData), nil | ||
| return crypto.Keccak256(rawData), rawData, nil |
There was a problem hiding this comment.
Correct Error Message in ComputeTypedDataAndHash
The error message on line 130 refers to "failed to pack and hash typedData EIP712Domain" when it should indicate an issue with the typedData.PrimaryType. This might cause confusion during debugging.
Apply this diff to correct the error message:
- return nil, nil, errors.Wrap(err, "failed to pack and hash typedData EIP712Domain")
+ return nil, nil, errors.Wrapf(err, "failed to pack and hash typedData %s", typedData.PrimaryType)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ComputeTypedDataAndHash computes the typed data and its keccak hash for signing | |
| func ComputeTypedDataAndHash(typedData TypedData) (hash, data []byte, err error) { | |
| domainSeparator, err := typedData.HashStruct("EIP712Domain", typedData.Domain.Map()) | |
| if err != nil { | |
| err = errors.Wrap(err, "failed to pack and hash typedData EIP712Domain") | |
| return nil, err | |
| return nil, nil, errors.Wrap(err, "failed to pack and hash typedData EIP712Domain") | |
| } | |
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) | |
| if err != nil { | |
| err = errors.Wrap(err, "failed to pack and hash typedData primary type") | |
| return nil, err | |
| return nil, nil, errors.Wrap(err, "failed to pack and hash typedData EIP712Domain") | |
| } | |
| rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash))) | |
| return crypto.Keccak256(rawData), nil | |
| return crypto.Keccak256(rawData), rawData, nil | |
| // ComputeTypedDataAndHash computes the typed data and its keccak hash for signing | |
| func ComputeTypedDataAndHash(typedData TypedData) (hash, data []byte, err error) { | |
| domainSeparator, err := typedData.HashStruct("EIP712Domain", typedData.Domain.Map()) | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "failed to pack and hash typedData EIP712Domain") | |
| } | |
| typedDataHash, err := typedData.HashStruct(typedData.PrimaryType, typedData.Message) | |
| if err != nil { | |
| return nil, nil, errors.Wrapf(err, "failed to pack and hash typedData %s", typedData.PrimaryType) | |
| } | |
| rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash))) | |
| return crypto.Keccak256(rawData), rawData, nil |
| chainCtx, _ := chainclient.NewClientContext("", "", nil) | ||
| protoCodec := codec.NewProtoCodec(chainCtx.InterfaceRegistry) | ||
|
|
There was a problem hiding this comment.
Initialize Codec Correctly for Testing
The test initializes a new ProtoCodec without registering all necessary interfaces, which might lead to incomplete or incorrect marshaling during the test. Ensure that all relevant message types are registered in the codec used for testing.
| legacytx.RegressionTestingAminoCodec = codec.NewLegacyAmino() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Using Global Variables in Tests
The assignment to legacytx.RegressionTestingAminoCodec affects global state and could interfere with other tests. Use a locally scoped codec within the test to prevent side effects.
masterbranchSummary by CodeRabbit
Release Notes
New Features
Improvements
Testing