fix: nft transfer packet spec #339
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a series of modifications across multiple files in the codebase, primarily focusing on package import restructuring, function signature updates, and minor logic adjustments. The changes span various modules including IBC hooks, NFT transfer, evidence, and Move-related components. Key modifications include simplifying function calls, updating import paths for the forwarding package, and enhancing error handling in certain methods. Changes
Poem
Suggested Reviewers
Possibly Related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 ReviewThe following issues were found:
License Issuesgo.mod
Denied Licenses: GPL-1.0-or-later, LGPL-2.0-or-later 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: 1
🧹 Nitpick comments (2)
x/ibc/nft-transfer/types/packet.go (2)
79-92: Consider adding error handling for JSON marshaling.While the implementation correctly handles token URIs and data reshaping, the use of
mustProtoMarshalJSONcould panic on marshaling errors.Consider using regular marshaling with error handling:
-return sdk.MustSortJSON(mustProtoMarshalJSON(&nftpd)) +bz, err := protoMarshalJSON(&nftpd) +if err != nil { + return nil +} +return sdk.MustSortJSON(bz)
115-122: Consider adding validation in DecodePacketData.While the decoding is correct, consider validating the decoded data using ValidateBasic.
func DecodePacketData(packetData []byte) (NonFungibleTokenPacketData, error) { var data NonFungibleTokenPacketData if err := unmarshalProtoJSON(packetData, &data); err != nil { return NonFungibleTokenPacketData{}, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) } + if err := data.ValidateBasic(); err != nil { + return NonFungibleTokenPacketData{}, err + } return data, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
client/docs/swagger-ui/swagger.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumx/ibc/nft-transfer/types/packet.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc/nft-transfer/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc/nft-transfer/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.go
📒 Files selected for processing (24)
app/ante/sigverify/sigverify.go(2 hunks)app/keepers/keepers.go(2 hunks)app/keepers/keys.go(1 hunks)app/modules.go(1 hunks)cmd/initiad/config.go(1 hunks)proto/ibc/applications/nft_transfer/v1/packet.proto(0 hunks)proto/ibc/applications/nft_transfer/v1/query.proto(1 hunks)x/evidence/keeper/grpc_query.go(1 hunks)x/ibc-hooks/move-hooks/hooks.go(3 hunks)x/ibc-hooks/move-hooks/message.go(2 hunks)x/ibc-hooks/move-hooks/receive.go(1 hunks)x/ibc-hooks/move-hooks/receive_test.go(0 hunks)x/ibc-hooks/move-hooks/util.go(1 hunks)x/ibc-hooks/move-hooks/util_test.go(1 hunks)x/ibc/nft-transfer/ibc_module.go(1 hunks)x/ibc/nft-transfer/keeper/relay.go(1 hunks)x/ibc/nft-transfer/types/codec.go(2 hunks)x/ibc/nft-transfer/types/packet.go(1 hunks)x/ibc/nft-transfer/types/packet_test.go(2 hunks)x/intertx/ibc_module.go(1 hunks)x/intertx/types/msgs.go(1 hunks)x/move/keeper/msg_server.go(1 hunks)x/move/keeper/vm_query.go(1 hunks)x/move/types/errors.go(1 hunks)
💤 Files with no reviewable changes (2)
- proto/ibc/applications/nft_transfer/v1/packet.proto
- x/ibc-hooks/move-hooks/receive_test.go
✅ Files skipped from review due to trivial changes (4)
- app/keepers/keys.go
- x/move/keeper/msg_server.go
- x/evidence/keeper/grpc_query.go
- x/intertx/types/msgs.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run test and upload codecov
- GitHub Check: Analyze (go)
🔇 Additional comments (26)
x/move/keeper/vm_query.go (1)
14-14: LGTM! Good use of named return values.The updated method signature with named return values improves code readability and self-documentation.
x/move/types/errors.go (1)
53-55: LGTM! Well-defined error registration.The new error variable is well-documented, follows the error code sequence, and has a clear error message.
x/ibc/nft-transfer/types/codec.go (4)
33-36: LGTM! Good practice using package-level variables for codec and registry.The initialization of
anyResolverandprotoCodecas package-level variables promotes reusability and follows the singleton pattern for these expensive-to-create objects.
57-72: LGTM! Well-structured JSON marshaling with proper interface handling.The implementation correctly:
- Uses
jsonpb.MarshalerwithOrigName: falsefor camel case output- Handles interface unpacking before marshaling
- Uses a buffer for efficient string building
74-77: LGTM! Simple and effective JSON unmarshaling.The function leverages the
protoCodecfor JSON unmarshaling, which is the recommended approach.
38-55: Consider handling panic recovery in production code.While panicking in case of marshaling errors might be acceptable during development, consider implementing proper error handling for production use.
-func mustProtoMarshalJSON(msg proto.Message) []byte { +func mustProtoMarshalJSON(msg proto.Message) ([]byte, error) { bz, err := protoMarshalJSON(msg, anyResolver) - if err != nil { - panic(err) - } - return bz + return bz, err }x/ibc-hooks/move-hooks/hooks.go (1)
40-40: LGTM! Simplified packet type checking.The removal of port parameters from
isIcs721Packetcalls streamlines the code while maintaining functionality. This change aligns with the PR objective of ensuring compliance with the NFT packet specification.Also applies to: 52-52, 64-64
cmd/initiad/config.go (1)
46-46: Verify the performance impact of disabling inter-block cache.Disabling the inter-block cache might affect performance. Please ensure this change is intentional and necessary.
x/ibc-hooks/move-hooks/message.go (1)
89-89: Verify the intention behind allowing maximum uint64 value.The change from
>to>=in the ID validation allowsuint64.MaxValueto be valid. Please confirm if this is intentional and doesn't introduce any edge cases.Also applies to: 98-98
✅ Verification successful
Validation change to disallow uint64.MaxValue is correct and improves safety
The change from
>to>=makes the validation more restrictive by disallowing IDs equal to uint64.MaxValue. This is a proper security measure to prevent potential overflow issues when working with uint64 IDs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests or documentation related to ID validation rg -A 5 "uint64\(0\)" -g "*.{go,md}" # Look for any existing uses of maximum uint64 value ast-grep --pattern 'uint64($v)'Length of output: 48502
x/ibc/nft-transfer/types/packet_test.go (2)
69-69: LGTM! Simplified packet data decoding.The removal of the port ID parameter aligns with the PR's objective of standardizing packet data encoding.
88-88: LGTM! Consistent packet data handling.The simplified GetBytes call maintains consistency with other changes in the codebase.
x/ibc/nft-transfer/types/packet.go (2)
66-73: LGTM! Improved token ID validation.The validation logic for token IDs is well-structured and handles both empty IDs and duplicates correctly.
95-112: LGTM! Well-structured helper function.The
requireShapehelper function is well-implemented with clear logic for determining when to set token URIs/data to nil.x/ibc-hooks/move-hooks/receive.go (1)
105-105: LGTM! Consistent packet data handling.The simplified GetBytes call aligns with the standardized packet data handling approach.
x/intertx/ibc_module.go (1)
4-4: LGTM! Updated to maintained protobuf library.The change from
github.com/gogo/protobuf/prototogithub.jparrowsec.cn/cosmos/gogoproto/protois a good practice as it uses a more actively maintained fork.x/ibc/nft-transfer/ibc_module.go (1)
182-182: Verify backward compatibility of packet data decoding changes.The removal of
sourcePortparameter fromDecodePacketDatasimplifies the packet handling, which aligns with NFT packet specification. However, we should ensure this change maintains compatibility with existing IBC connections.Run the following script to check for any existing IBC connections that might be affected:
✅ Verification successful
✓ Packet data decoding change is safe and specification-compliant
The removal of
sourcePortparameter fromDecodePacketDatais verified to be backward compatible:
- The change aligns with ICS-721 NFT transfer specification
- Version compatibility is enforced at channel level with strict checks
- Existing tests already use the simplified signature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for active IBC connections and their packet data format # Search for IBC connection configurations rg -A 5 'IBC.*Connection' # Search for packet data decoding patterns ast-grep --pattern 'DecodePacketData($$$)'Length of output: 2188
Script:
#!/bin/bash # Check packet data structure and version handling # Search for packet data structure definition ast-grep --pattern 'type PacketData struct { $$$ }' # Look for version-related code in packet handling rg -A 5 'Version|version' x/ibc/nft-transfer/ # Check for packet validation logic ast-grep --pattern 'func $_(PacketData $$$) { $$$ }'Length of output: 14578
x/ibc/nft-transfer/keeper/relay.go (1)
222-222: Consistent packet data serialization change.The removal of
destinationPortfromGetBytescall aligns with the packet decoding changes, ensuring consistent packet data handling throughout the IBC transfer path.app/modules.go (1)
86-87: LGTM! Import path simplification.The forwarding package import paths have been simplified, improving code organization without affecting functionality.
app/ante/sigverify/sigverify.go (2)
174-174: Function name follows camelCase convention.The rename from
MultisignaturetoMultiSignatureimproves naming consistency.
180-182: Verify gas consumption for forwarding public keys.The new case for
ForwardingPubKeyreturnsnilwithout consuming gas. Ensure this is the intended behavior for forwarded transactions.Run the following script to check for similar gas consumption patterns:
✅ Verification successful
Verified: No gas consumption for forwarding public keys is intentional
The current implementation follows the codebase's pattern where each public key type either consumes gas or explicitly skips it. The
ForwardingPubKeycase returnsnilwithout gas consumption, which is likely by design as the actual signature verification (and associated gas costs) would be handled within the forwarding module itself.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check gas consumption patterns for different public key types # Search for gas consumption patterns in signature verification ast-grep --pattern 'meter.ConsumeGas($$$)'Length of output: 44
Script:
#!/bin/bash # First, verify the file exists and check its content echo "=== Checking file content ===" cat app/ante/sigverify/sigverify.go echo -e "\n=== Searching for gas-related patterns ===" rg -i "gas" app/ante/sigverify/sigverify.go echo -e "\n=== Looking for other public key type handlers ===" rg "case \*" app/ante/sigverify/sigverify.goLength of output: 8478
app/keepers/keepers.go (2)
107-109: LGTM! Import paths have been simplified.The import paths for the forwarding package have been simplified, making the code more maintainable.
644-645: LGTM! Request type has been corrected.The request type for GetAllCurrencyPairs query has been correctly updated to use
GetAllCurrencyPairsRequestinstead ofGetAllCurrencyPairsResponse.proto/ibc/applications/nft_transfer/v1/query.proto (2)
26-26: LGTM! HTTP path has been improved.The HTTP GET path has been enhanced with:
- Corrected spelling from "class_hashs" to "class_hashes"
- Added wildcard parameter
{trace=**}for more flexible matching
31-31: LGTM! HTTP path has been enhanced.The HTTP GET path has been improved by adding a wildcard parameter
{channel_id=**}for more flexible channel ID matching.x/ibc-hooks/move-hooks/util_test.go (1)
37-37: LGTM! Test cases updated to match simplified function signature.The test cases have been correctly updated to call
isIcs721Packetwithout thecounterpartyPortparameter, aligning with the function signature changes.Also applies to: 43-43
x/ibc-hooks/move-hooks/util.go (1)
41-45: LGTM! Function signature has been simplified.The
isIcs721Packetfunction has been improved by:
- Removing unnecessary
counterpartyPortparameter- Streamlining the return statements for better readability
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
==========================================
- Coverage 41.22% 41.14% -0.08%
==========================================
Files 269 269
Lines 25734 25737 +3
==========================================
- Hits 10608 10589 -19
- Misses 13488 13504 +16
- Partials 1638 1644 +6
|
Description
--
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...