feat!: create account if NFT recipient account does not exist #319
Conversation
📝 WalkthroughWalkthroughThe changes introduced in this pull request involve modifications to the NFT transfer functionality within the IBC module. Key updates include adjustments to the Changes
Sequence Diagram(s)sequenceDiagram
participant Sender
participant Receiver
participant Keeper
participant AccountKeeper
Sender->>Keeper: Send NFT
Keeper->>AccountKeeper: Check if Receiver account exists
alt Account does not exist
AccountKeeper-->>Keeper: Account does not exist
Keeper->>AccountKeeper: Create new account for Receiver
AccountKeeper-->>Keeper: Account created
end
Keeper->>Receiver: Transfer NFT
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 |
beer-1
changed the title
![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
🧹 Outside diff range and nitpick comments (1)
x/ibc/nft-transfer/keeper/integration_test.go (1)
230-260: Enhance test coverage for non-existing account scenarioWhile the test case verifies basic functionality, consider adding these scenarios:
- Verify account properties after creation
- Test with multiple non-existing accounts
- Add error cases for account creation failures
Example enhancement:
suite.Run("transfer A->B (non-existing account)", func() { { sender := pathA2B.EndpointA.Chain.SenderAccount.GetAddress() // random receiver account receiver := make(sdk.AccAddress, 20) _, err := rand.Read(receiver) suite.NoError(err) packet = suite.transferNft( pathA2B.EndpointA, pathA2B.EndpointB, targetClassId, nftId, sender.String(), receiver.String(), ) targetClassId = suite.receiverNft( pathA2B.EndpointA, pathA2B.EndpointB, packet, ) suite.ConfirmClassId(pathA2B.EndpointB, classId, targetClassId) // receiver account should be created suite.True(suite.chainB.App.GetAccountKeeper().HasAccount(suite.chainB.GetContext(), receiver)) + // verify account properties + acc := suite.chainB.App.GetAccountKeeper().GetAccount(suite.chainB.GetContext(), receiver) + suite.NotNil(acc) + suite.Equal(receiver, acc.GetAddress()) } }) +// Test multiple non-existing accounts +suite.Run("transfer A->B (multiple non-existing accounts)", func() { + for i := 0; i < 3; i++ { + receiver := make(sdk.AccAddress, 20) + _, err := rand.Read(receiver) + suite.NoError(err) + + // Verify account doesn't exist + suite.False(suite.chainB.App.GetAccountKeeper().HasAccount(suite.chainB.GetContext(), receiver)) + + // Perform transfer + packet = suite.transferNft(...) + + // Verify account was created + suite.True(suite.chainB.App.GetAccountKeeper().HasAccount(suite.chainB.GetContext(), receiver)) + } +})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
x/ibc/nft-transfer/keeper/integration_test.go(3 hunks)x/ibc/nft-transfer/keeper/relay.go(1 hunks)x/ibc/nft-transfer/types/expected_keeper.go(1 hunks)
🔇 Additional comments (2)
x/ibc/nft-transfer/types/expected_keeper.go (1)
22-24: LGTM: Well-defined interface methods for account management
The new methods added to the AccountKeeper interface provide the necessary functionality for managing accounts in a clean and modular way.
x/ibc/nft-transfer/keeper/integration_test.go (1)
4-5: LGTM: Appropriate imports for functionality
The import changes are appropriate:
- Using crypto/rand for secure random number generation
- Consistent sdk alias usage
Also applies to: 9-9
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #319 +/- ##
=======================================
Coverage 41.14% 41.14%
=======================================
Files 269 269
Lines 25699 25702 +3
=======================================
+ Hits 10573 10576 +3
Misses 13488 13488
Partials 1638 1638
|
Description
This PR contains consensus breaking changes, so should be shipped with upgrade.
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...