Fix errors on amino conversions #100
Conversation
WalkthroughThis pull request introduces a series of consistent changes across multiple message classes in the WebAssembly (Wasm) and Inter-Blockchain Communication (IBC) modules. The primary modifications involve refining how message properties are handled, particularly the Changes
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
CodeRabbit Configuration File (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (9)
src/core/wasm/msgs/MsgInstantiateContractV2.ts (1)
158-158: Improve type safety for msg propertyUsing the generic
JSONtype might be too permissive and could lead to runtime errors.Consider defining a more specific type for the message structure:
export interface WasmMsg { [key: string]: unknown; // Add any known common fields here } // Then use it in interfaces interface Amino { // ... msg: WasmMsg; } interface Data { // ... msg: WasmMsg; }This provides better type safety and documentation of the expected message structure.
Also applies to: 171-171
src/core/wasm/msgs/MsgStoreAndInstantiateContract.ts (3)
143-143: Reduce code duplication in JSON to base64 conversion.This code is identical to the conversion in
fromAmino. Consider extracting it to a private helper method:+ private static encodeMsg(msg: JSON): string { + if (!msg) return ''; + try { + return Buffer.from(JSON.stringify(msg)).toString('base64'); + } catch (e) { + throw new Error(`Invalid msg format: ${e.message}`); + } + } + public static fromAmino(...) { - Buffer.from(JSON.stringify(msg)).toString('base64'), + MsgStoreAndInstantiateContract.encodeMsg(msg), } public static fromData(...) { - Buffer.from(JSON.stringify(msg)).toString('base64'), + MsgStoreAndInstantiateContract.encodeMsg(msg), }
174-174: Reduce code duplication in base64 to JSON conversion.This code is identical to the conversion in
toAmino. Consider extracting it to a private helper method:+ private static decodeMsg(msg: string): JSON { + if (!msg) return null; + try { + const decoded = Buffer.from(msg, 'base64').toString(); + return JSON.parse(decoded); + } catch (e) { + throw new Error(`Invalid base64 or JSON format: ${e.message}`); + } + } + public toAmino(...) { - msg: JSON.parse(Buffer.from(msg, 'base64').toString()), + msg: MsgStoreAndInstantiateContract.decodeMsg(msg), } public toData(...) { - msg: JSON.parse(Buffer.from(msg, 'base64').toString()), + msg: MsgStoreAndInstantiateContract.decodeMsg(msg), }
256-256: Consider using a more specific type than JSON.The
JSONtype in TypeScript is quite permissive. Consider defining a more specific type or interface that accurately represents the expected message structure:export interface ContractMsg { // Define the expected message structure here // Example: action: string; params?: Record<string, unknown>; } // Then use it in the interfaces: msg: ContractMsg;Also applies to: 272-272
src/core/wasm/msgs/MsgExecuteContract.ts (3)
Line range hint
19-31: Update constructor documentation and types to reflect base64 encoding.The constructor's documentation and type definition should be updated to clearly indicate that
msgis expected to be a base64-encoded JSON string./** * @param sender the actor that signed the messages * @param contract the address of the smart contract - * @param msg json encoded message to be passed to the contract + * @param msg base64-encoded JSON message to be passed to the contract * @param funds coins that are transferred to the contract on execution */ constructor( public sender: AccAddress, public contract: AccAddress, - public msg: string, + public msg: string, // base64-encoded JSON string funds: Coins.Input )
61-66: Reduce code duplication in encoding/decoding operations.The JSON stringification and base64 encoding/decoding logic is duplicated across methods. Consider extracting these operations into utility methods.
+ private static encodeMsg(msg: JSON): string { + try { + return Buffer.from(JSON.stringify(msg)).toString('base64') + } catch (e) { + throw new Error(`Failed to encode message: ${e.message}`) + } + } + + private static decodeMsg(msg: string): JSON { + try { + return JSON.parse(Buffer.from(msg, 'base64').toString()) + } catch (e) { + throw new Error(`Failed to decode message: ${e.message}`) + } + } public static fromData(data: MsgExecuteContract.Data): MsgExecuteContract { const { sender, contract, msg, funds } = data return new MsgExecuteContract( sender, contract, - Buffer.from(JSON.stringify(msg)).toString('base64'), + MsgExecuteContract.encodeMsg(msg), Coins.fromData(funds) ) } public toData(): MsgExecuteContract.Data { return { '@type': '/cosmwasm.wasm.v1.MsgExecuteContract', sender, contract, - msg: JSON.parse(Buffer.from(msg, 'base64').toString()), + msg: MsgExecuteContract.decodeMsg(msg), funds: funds.toData(), } }Also applies to: 75-75
119-119: Consider using a more specific type thanJSON.The
JSONtype is not a standard TypeScript type and might be too permissive. Consider using a more specific type or interface that describes the expected structure of the message.- msg: JSON + msg: Record<string, unknown>Also applies to: 128-128
src/core/wasm/msgs/MsgSudoContract.ts (1)
105-105: Strengthen type safety for msg propertyUsing the
JSONtype is too loose and might lead to runtime errors. Consider using a more specific type or interface that describes the expected structure of the message.Consider this approach:
+ // Define a type that represents the expected message structure + export interface SudoMsg { + [key: string]: unknown; // Or define specific message fields + } export interface Amino { type: 'wasm/MsgSudoContract' value: { authority: AccAddress contract: AccAddress - msg: JSON + msg: SudoMsg } } export interface Data { '@type': '/cosmwasm.wasm.v1.MsgSudoContract' authority: AccAddress contract: AccAddress - msg: JSON + msg: SudoMsg }Also applies to: 113-113
src/core/wasm/msgs/MsgInstantiateContract.ts (1)
142-142: Consider using a more specific type than JSONWhile changing the type from
stringtoJSONbetter represents the data structure, using theJSONtype might be too permissive as it allows any valid JSON value.Consider defining a more specific type or interface that represents the expected message structure:
export namespace MsgInstantiateContract { // Define a specific message type export type ContractMsg = Record<string, unknown>; export interface Amino { // ... msg: ContractMsg; // ... } export interface Data { // ... msg: ContractMsg; // ... } }This would provide better type safety and documentation of the expected message structure.
Also applies to: 153-153
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.json(1 hunks)src/core/ibc/applications/nft-transfer/msgs/MsgNftTransfer.ts(3 hunks)src/core/ibc/applications/transfer/msgs/MsgTransfer.ts(3 hunks)src/core/wasm/msgs/MsgExecuteContract.ts(5 hunks)src/core/wasm/msgs/MsgInstantiateContract.ts(6 hunks)src/core/wasm/msgs/MsgInstantiateContractV2.ts(6 hunks)src/core/wasm/msgs/MsgMigrateContract.ts(5 hunks)src/core/wasm/msgs/MsgStoreAndInstantiateContract.ts(6 hunks)src/core/wasm/msgs/MsgStoreAndMigrateContract.ts(6 hunks)src/core/wasm/msgs/MsgSudoContract.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments (13)
src/core/wasm/msgs/MsgInstantiateContractV2.ts (1)
Line range hint
1-177: Verify consistency across other Wasm message typesLet's ensure these changes are consistent with other Wasm message types mentioned in the summary.
✅ Verification successful
Base64/JSON handling is consistent across all Wasm message types
The implementation in MsgInstantiateContractV2.ts follows the same pattern used consistently across all other Wasm message types for handling msg serialization/deserialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if similar changes are needed in other Wasm message files echo "Checking other Wasm message files for consistency..." # Look for similar message handling patterns rg -l "msg.*JSON" "src/core/wasm/msgs/" # Check for potential inconsistencies in base64/JSON handling rg -A 2 "Buffer.from.*base64" "src/core/wasm/msgs/"Length of output: 12246
src/core/wasm/msgs/MsgMigrateContract.ts (3)
57-63: Apply consistent error handling across fromAmino and fromDataThe same concerns about validation and error handling apply here as in the
fromAminomethod.Please apply the same improvements suggested for the
fromAminomethod.
73-73: Apply consistent error handling across toAmino and toDataThe same concerns about base64 decoding and JSON parsing apply here as in the
toAminomethod.Please apply the same improvements suggested for the
toAminomethod.
Line range hint
34-73: Verify security implications of message encoding/decodingSince this code handles contract migration messages, which could contain sensitive data or executable code:
- Verify that the base64 encoding/decoding doesn't introduce any security vulnerabilities
- Ensure that JSON parsing is safe against injection attacks
- Consider adding input validation for the contract code_id
Run the following script to check for similar message handling patterns and potential security issues:
✅ Verification successful
Implementation follows secure message handling patterns
The code follows consistent and secure patterns for message handling across the codebase:
- Uses standard Node.js Buffer API for base64 encoding/decoding
- Consistent with Cosmos SDK's protocol-level message validation
- Matches established patterns used in other core modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar message encoding patterns echo "Checking message encoding patterns..." rg -l "Buffer.from\(.*\).toString\('base64'\)" --type ts # Search for JSON.parse usage without try-catch echo "Checking unsafe JSON parsing..." rg "JSON\.parse\(" --type ts -C 2 | grep -v "try" # Search for potential validation patterns echo "Checking validation patterns..." rg "code_id.*parseInt" --type ts -C 2Length of output: 11913
src/core/wasm/msgs/MsgExecuteContract.ts (1)
Line range hint
1-134: Verify compatibility with message consumers.The change to base64-encoded JSON messages might affect other components that interact with this class. Let's verify the usage:
✅ Verification successful
Base64 encoding changes are properly encapsulated.
The verification shows that all interactions with the message field are handled through the class's public interface methods, which properly manage the base64 encoding/decoding internally. The pattern is consistent with other contract-related messages in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of MsgExecuteContract rg -l "new MsgExecuteContract\(" | while read -r file; do echo "=== $file ===" rg -A 5 "new MsgExecuteContract\(" "$file" done # Search for message handling in contract execution rg -l "MsgExecuteContract.*msg" | while read -r file; do echo "=== $file ===" rg -A 5 "MsgExecuteContract.*msg" "$file" doneLength of output: 1083
Script:
#!/bin/bash # Search for direct access to msg field rg -A 3 "\.msg\b" --type ts # Search for test files to verify expected behavior fd -e test.ts -e spec.ts | while read -r file; do echo "=== $file ===" rg "MsgExecuteContract" "$file" doneLength of output: 17233
src/core/wasm/msgs/MsgSudoContract.ts (1)
Line range hint
1-116: Verify consistent message handling across the codebaseLet's verify that these changes are consistent with other message classes and their usage.
Run this script to check for consistency:
✅ Verification successful
Implementation verified as consistent with codebase patterns
The message handling in
MsgSudoContractfollows the same patterns used across all wasm message classes for base64 encoding/decoding and JSON serialization/deserialization of themsgfield.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent message handling patterns across related files # Check other message classes in the wasm module echo "Checking other message classes for consistent patterns..." ast-grep --pattern 'class $_ extends JSONSerializable { $$$ msg: string $$$ }' # Check for usage of MsgSudoContract echo "Checking usage of MsgSudoContract..." rg "new MsgSudoContract\(" -A 3Length of output: 1158
Script:
#!/bin/bash # Find all wasm-related message classes and their patterns # Check for all wasm message classes echo "Checking wasm message classes..." fd "Msg.*\.ts$" src/core/wasm # Check base64 encoding patterns in wasm messages echo -e "\nChecking base64 encoding patterns..." rg "toString\('base64'\)" src/core/wasm -B 1 -A 1 # Check JSON parsing patterns echo -e "\nChecking JSON parsing patterns..." rg "JSON\.parse\(" src/core/wasm -B 1 -A 1Length of output: 10979
src/core/wasm/msgs/MsgInstantiateContract.ts (2)
Line range hint
1-159: Reconsider the approach to message serializationThe current changes introduce base64 encoding/decoding cycles that seem unnecessary and could potentially cause issues. The PR aims to fix Amino conversion errors, but the introduced changes might add complexity without solving the underlying problems.
Consider the following architectural recommendations:
- Keep JSON messages in their native format for Amino/Data serialization
- Only use base64 encoding when absolutely necessary (e.g., for Proto serialization)
- Add validation for message structure during serialization/deserialization
- Consider adding test cases that verify message integrity through the full serialization cycle
Let's verify the impact of these changes:
48-48:⚠️ Potential issueSimplify msg conversion in Amino serialization
The current implementation introduces unnecessary encoding/decoding cycles that could potentially cause issues:
fromAmino: Converting JSON to base64 is unnecessary since the msg is already in JSON formattoAmino: The base64 decoding step is redundant and could be simplifiedConsider simplifying the conversion:
// in fromAmino - Buffer.from(JSON.stringify(msg)).toString('base64'), + JSON.stringify(msg), // in toAmino - msg: JSON.parse(Buffer.from(msg, 'base64').toString()), + msg: JSON.parse(msg),Let's verify if this pattern is consistent across other Wasm message types:
Also applies to: 62-62
src/core/wasm/msgs/MsgStoreAndMigrateContract.ts (1)
Line range hint
1-167: Verify the impact on dependent code.These changes to msg handling might affect other parts of the system that interact with MsgStoreAndMigrateContract.
Let's check for potential impacts:
✅ Verification successful
Changes are safe and localized to internal implementation
The modifications to msg handling are encapsulated within the class and don't affect its public interface. External usage in
core/Msg.tsandmsgs/index.tsonly deals with message type conversion and routing, which remain unchanged.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of MsgStoreAndMigrateContract to verify compatibility # Find direct usage of the class echo "Direct usage of MsgStoreAndMigrateContract:" rg "new MsgStoreAndMigrateContract\(" -A 5 # Find places where msg is accessed echo "\nAccessing msg property:" rg "MsgStoreAndMigrateContract.*\.msg" -A 5 # Find test files that might need updates echo "\nRelated test files:" fd "test.*MsgStoreAndMigrateContract.*\.(ts|js|tsx|jsx)$"Length of output: 1863
Script:
#!/bin/bash # Description: Find imports and references to MsgStoreAndMigrateContract # Find imports of MsgStoreAndMigrateContract echo "Imports of MsgStoreAndMigrateContract:" rg "import.*MsgStoreAndMigrateContract" --type ts # Find any string references to MsgStoreAndMigrateContract echo "\nString references to MsgStoreAndMigrateContract:" rg "MsgStoreAndMigrateContract" --type tsLength of output: 4512
src/core/ibc/applications/transfer/msgs/MsgTransfer.ts (3)
115-117: Improve handling of empty valuesGood improvements in handling empty/undefined values:
- Using nullish coalescing for
timeout_heightensures consistent object output- Converting empty memo string to undefined maintains cleaner serialization
241-241:⚠️ Potential issueBreaking Change: timeout_height is now required in Amino interface
Making
timeout_heightrequired in the Amino interface is a breaking change that could affect existing consumers. Please ensure this change is intentional and documented.
90-90: Verify error handling for undefined timeout_heightThe change to always call
Height.fromAmino(timeout_height)could throw an error iftimeout_heightis undefined. This seems inconsistent with the error check at line 77 which allows eithertimeout_heightortimeout_timestampto be defined.src/core/ibc/applications/nft-transfer/msgs/MsgNftTransfer.ts (1)
95-95:⚠️ Potential issueConsistent changes with MsgTransfer
The changes in
MsgNftTransfermirror those inMsgTransfer:
- Similar handling of
timeout_heightinfromAmino- Identical treatment of empty values in
toAmino- Same interface changes making
timeout_heightrequiredWhile consistency is good, both classes share the same concerns:
- Potential error handling issues with undefined
timeout_height- Breaking change in the Amino interface
Also applies to: 122-124, 256-256
✅ Verification successful
Changes are safe and maintain module consistency
The modifications to
timeout_heighthandling inMsgNftTransferare well-contained:
- Limited to the NFT transfer module's internal implementation
- No external implementations of the Amino interface found
- Maintains consistency with the
MsgTransferimplementation🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Compare usage patterns between MsgTransfer and MsgNftTransfer echo "=== MsgNftTransfer usage ===" rg -A 5 "MsgNftTransfer\.fromAmino" # Search for potential implementations or extensions echo "=== Interface implementations ===" ast-grep --pattern 'interface $_ extends MsgNftTransfer.Amino'Length of output: 541
Script:
#!/bin/bash # Check the implementation of fromAmino and related methods echo "=== MsgNftTransfer implementation ===" ast-grep --pattern 'class MsgNftTransfer { $$$ static fromAmino($_) { $$$ } $$$ }' # Look for other NFT transfer related handlers echo "=== Related NFT handlers ===" rg "nft-transfer/Msg" -A 2Length of output: 1647
Script:
#!/bin/bash # Check Height class implementation echo "=== Height implementation ===" ast-grep --pattern 'class Height { $$$ static fromAmino($_) { $$$ } $$$ }' # Look for direct consumers of the Amino interface echo "=== Amino interface usage ===" rg "MsgNftTransfer\.Amino" -A 2Length of output: 1445
Summary by CodeRabbit
Version Update
Core Changes
timeout_heightin IBC transfer and NFT transfer messagesTechnical Improvements