-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat(kvindexer): add evm-nft submodule #133
Conversation
WalkthroughThis pull request introduces NFT (Non-Fungible Token) indexing functionality to the application. The changes primarily involve adding a new NFT submodule to the indexer in Changes
Sequence DiagramsequenceDiagram
participant App as MinitiaApp
participant Indexer as KVIndexer
participant NFTSubmodule as NFT Submodule
App->>Indexer: Initialize Indexer
Indexer->>NFTSubmodule: Create NFT Submodule
NFTSubmodule-->>Indexer: Submodule Created
Indexer->>Indexer: Register NFT Submodule
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
==========================================
- Coverage 57.04% 57.03% -0.02%
==========================================
Files 110 110
Lines 10045 10049 +4
==========================================
+ Hits 5730 5731 +1
- Misses 3493 3495 +2
- Partials 822 823 +1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/docs/swagger-ui/swagger.yaml (2)
43083-43642
: LGTM: Well-designed NFT endpoints with standard paginationThe new NFT endpoints follow REST conventions and include standard cosmos-sdk pagination parameters.
Fix typo in the summary of TokensByAccount endpoint:
- summary: TokensByACcount queries all the NFTs of an account + summary: TokensByAccount queries all the NFTs of an account
70832-71091
: LGTM: Well-structured NFT data modelsThe NFT data models are well-organized and follow a clear hierarchy with appropriate relationships between collections and tokens.
Consider adding examples in the documentation for common use cases of these models.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (4)
app/indexer.go
(2 hunks)client/docs/config.json
(1 hunks)client/docs/swagger-ui/swagger.yaml
(8 hunks)go.mod
(1 hunks)
🔇 Additional comments (7)
client/docs/swagger-ui/swagger.yaml (2)
37511-37511
: LGTM: Documentation improvements
The description text improvements enhance clarity across multiple definitions.
Also applies to: 68008-68008, 68141-68141, 68919-68919
37226-37226
: Breaking Change: Verify impact of base_denom endpoint modification
The endpoint /opinit/opchild/v1/base_denom/{denom}
has been modified:
- Path parameter
denom
is now a query parameter - Parameter is now optional (required: false)
This is a breaking change that will require updates to existing API clients.
Also applies to: 37436-37437
app/indexer.go (3)
16-16
: LGTM!
The import follows the established pattern for submodule imports.
64-64
: LGTM!
The submodule registration follows the established pattern and maintains proper error handling.
60-63
: Verify NFT submodule dependencies
The initialization looks correct with proper error handling. However, let's verify the dependency on smPair
.
✅ Verification successful
NFT submodule dependency on smPair is correctly initialized and registered
The verification shows that:
smPair
is properly initialized before being passed toNewEvmNFTSubmodule
- Both
smPair
andsmNft
are correctly registered with thekvIndexerKeeper
- The dependency chain is maintained in the proper order: pair submodule is created first, then used in NFT submodule initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how smPair is used within the NFT submodule
# Search for pair-related function calls in NFT submodule
ast-grep --pattern 'smPair.$_($$$)'
Length of output: 37
Script:
#!/bin/bash
# Let's try a different approach to understand the NFT submodule dependencies
# First, let's find the NFT submodule implementation
fd "nft" --type f --exec rg -l "NewEvmNFTSubmodule"
# Then look for any usage of pair-related functionality
fd "nft" --type f --exec rg -l "smPair"
# Finally, let's examine the NFT submodule implementation
ast-grep --pattern 'func NewEvmNFTSubmodule($$$) {
$$$
}'
Length of output: 149
Script:
#!/bin/bash
# Let's search for the NFT module implementation files more broadly
fd -e go -e sol --type f --exec rg -l "NFTSubmodule"
# Then let's look at the actual implementation
rg -A 10 "NewEvmNFTSubmodule"
# Also search for any pair-related dependencies
rg "smPair" -A 5 -B 5
Length of output: 1486
client/docs/config.json (1)
195-198
: LGTM!
The NFT query API endpoint is properly added following the established pattern for indexer endpoints.
go.mod (1)
43-43
: Verify version compatibility with other kvindexer submodules
The new dependency is properly added. Let's verify version compatibility with other kvindexer submodules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Closes: -
added kvindexer's evm-nft submodule
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...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores