feat: add coin types to core #23346
feat: add coin types to core #23346
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant MsgServer
participant Keeper
participant CoreCoin
Client->>MsgServer: Send transaction
MsgServer->>CoreCoin: Convert coins to core.Coins
CoreCoin-->>MsgServer: Converted coins
MsgServer->>Keeper: Call SendCoins with core.Coins
Keeper->>Keeper: Process coin transfer
Possibly related PRs
Suggested Labels
Suggested Reviewers
Finishing Touches
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
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: 2
🧹 Nitpick comments (2)
x/bank/keeper/msg_server.go (1)
60-68: Refactor coin conversion for clarity and efficiency.Converting
msg.AmounttocoreAmountusing a loop can be refactored for better readability. Consider using a helper function to handle the conversion.Example helper function:
func sdkCoinsToCoreCoins(sdkCoins sdk.Coins) core.Coins { coreCoins := make(core.Coins, len(sdkCoins)) for i, coin := range sdkCoins { coreCoins[i] = core.Coin{ Denom: coin.Denom, Amount: *coin.Amount.BigInt(), } } return coreCoins }Updated code:
var coreAmount core.Coins -for _, coin := range msg.Amount { - coreAmount = append(coreAmount, core.Coin{ - Denom: coin.Denom, - Amount: *coin.Amount.BigInt(), - }) -} +coreAmount := sdkCoinsToCoreCoins(msg.Amount)types/coin.go (1)
212-223: Optimize slice allocation in NewCoreCoinsThe current implementation grows the slice dynamically. Pre-allocating the slice would improve performance by avoiding reallocations.
Apply this diff to optimize the function:
func NewCoreCoins(coins core.Coins) Coins { - var res Coins + res := make(Coins, 0, len(coins)) for _, coin := range coins { res = append(res, Coin{ Denom: coin.Denom, Amount: math.NewIntFromBigInt(&coin.Amount), }) } return NewCoins(res...) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
core/coin/coin.go(1 hunks)go.mod(1 hunks)simapp/v2/go.mod(1 hunks)types/coin.go(3 hunks)x/bank/go.mod(1 hunks)x/bank/keeper/keeper.go(5 hunks)x/bank/keeper/msg_server.go(2 hunks)x/bank/keeper/send.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/coin/coin.go
🧰 Additional context used
📓 Path-based instructions (4)
x/bank/keeper/msg_server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
types/coin.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/keeper/send.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/keeper/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 GitHub Actions: CodeQL
go.mod
[warning] go mod tidy -e altered go.mod. Changes need to be checked into version control
x/bank/go.mod
[warning] Multiple go.mod files were modified after running 'go mod tidy -e'. Changes need to be reviewed and checked into version control.
simapp/v2/go.mod
[warning] Multiple go.mod files were modified after running 'go mod tidy -e'. Changes need to be reviewed and checked into version control.
types/coin.go
[error] 12-12: Missing required module dependency: package cosmossdk.io/core/coin is not provided by any module
🪛 GitHub Check: dependency-review
types/coin.go
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
🪛 GitHub Actions: Dependency Review
types/coin.go
[error] 12-12: Missing required module: no required module provides package cosmossdk.io/core/coin
🪛 GitHub Actions: Tests / Code Coverage
types/coin.go
[error] 12-12: Missing required module: no required module provides package cosmossdk.io/core/coin
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (7)
x/bank/keeper/send.go (2)
30-30: Method signature updated appropriately.The
SendCoinsmethod now correctly acceptsamtof typecore.Coins, aligning with the updated coin types in the system.
201-203: Verify the conversion functionsdk.NewCoreCoins.The function
sdk.NewCoreCoins(rawAmt)is used to convertcore.Coinstosdk.Coins. Please ensure that this function exists and correctly handles the conversion, including validation and handling of edge cases.x/bank/keeper/keeper.go (2)
43-45: Ensure all implementations align with updated interface methods.The methods now accept
amtof typecore.Coins. Verify that all implementations and calls to these methods are updated accordingly throughout the codebase.
255-257: Check internal handling ofcore.Coinsin method bodies.In the implementations of
SendCoinsFromModuleToAccount,SendCoinsFromModuleToModule, andSendCoinsFromAccountToModule, ensure thatamtis correctly processed ascore.Coins, including any necessary conversions tosdk.Coinswithin the method logic.Also applies to: 272-274, 290-292
x/bank/go.mod (1)
172-173: LGTM: Replace directive correctly addedThe replace directive for the core module is properly configured to use the local version.
🧰 Tools
🪛 GitHub Actions: CodeQL
[warning] Multiple go.mod files were modified after running 'go mod tidy -e'. Changes need to be reviewed and checked into version control.
go.mod (1)
179-179: LGTM: Core module replacement properly configuredThe replace directive is correctly added to the appropriate section, maintaining consistency with other module replacements.
🧰 Tools
🪛 GitHub Actions: CodeQL
[warning] go mod tidy -e altered go.mod. Changes need to be checked into version control
simapp/v2/go.mod (1)
286-286: LGTM: Core module replacement properly configuredThe replace directive is correctly added to the SimApp SDK modules section, maintaining consistency with other module replacements.
🧰 Tools
🪛 GitHub Actions: CodeQL
[warning] Multiple go.mod files were modified after running 'go mod tidy -e'. Changes need to be reviewed and checked into version control.
| @@ -9,6 +9,7 @@ | |||
| "strings" | |||
| "unicode" | |||
|
|
|||
| core "cosmossdk.io/core/coin" | |||
There was a problem hiding this comment.
Add core module dependency to go.mod
The import of cosmossdk.io/core/coin requires adding the module to go.mod. This is currently causing build failures.
Add the following to your go.mod file:
require (
+ cosmossdk.io/core v1.0.0
)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: dependency-review
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
🪛 GitHub Actions: Dependency Review
[error] 12-12: Missing required module: no required module provides package cosmossdk.io/core/coin
🪛 GitHub Actions: CodeQL
[error] 12-12: Missing required module dependency: package cosmossdk.io/core/coin is not provided by any module
🪛 GitHub Actions: Tests / Code Coverage
[error] 12-12: Missing required module: no required module provides package cosmossdk.io/core/coin
| func NewCoreCoin(coin core.Coin) Coin { | ||
| return Coin{ | ||
| Denom: coin.Denom, | ||
| Amount: math.NewIntFromBigInt(&coin.Amount), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation in NewCoreCoin
The function directly converts core.Coin to Coin without validation. Consider validating the denomination and amount to maintain data integrity.
Apply this diff to add validation:
func NewCoreCoin(coin core.Coin) Coin {
- return Coin{
+ c := Coin{
Denom: coin.Denom,
Amount: math.NewIntFromBigInt(&coin.Amount),
}
+ if err := c.Validate(); err != nil {
+ panic(err)
+ }
+ return c
}📝 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.
| func NewCoreCoin(coin core.Coin) Coin { | |
| return Coin{ | |
| Denom: coin.Denom, | |
| Amount: math.NewIntFromBigInt(&coin.Amount), | |
| } | |
| } | |
| func NewCoreCoin(coin core.Coin) Coin { | |
| c := Coin{ | |
| Denom: coin.Denom, | |
| Amount: math.NewIntFromBigInt(&coin.Amount), | |
| } | |
| if err := c.Validate(); err != nil { | |
| panic(err) | |
| } | |
| return c | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
x/bank/keeper/send.go (1)
201-203: Consider adding validation before conversionWhile the conversion looks correct, consider validating rawAmt before conversion to ensure all amounts are non-nil and positive.
func (k BaseSendKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccAddress, rawAmt core.Coins) error { + for _, coin := range rawAmt { + if coin.Amount.Sign() < 0 { + return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, "negative coin amount") + } + } amt := sdk.NewCoreCoins(rawAmt)types/coin.go (2)
34-39: Add validation in NewCoreCoinConsider adding validation for the input core.Coin to ensure we're not creating invalid coins.
func NewCoreCoin(coin core.Coin) Coin { + if len(coin.Denom) == 0 { + panic("empty denom") + } return Coin{ Denom: coin.Denom, Amount: math.NewIntFromBigInt(&coin.Amount), } }
212-223: Consider optimizing NewCoreCoins allocationThe current implementation could be optimized by pre-allocating the result slice.
func NewCoreCoins(coins core.Coins) Coins { - var res Coins + res := make(Coins, 0, len(coins)) for _, coin := range coins { res = append(res, Coin{ Denom: coin.Denom, Amount: math.NewIntFromBigInt(&coin.Amount), }) } return NewCoins(res...) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
core/coin/coin.go(1 hunks)go.mod(1 hunks)simapp/v2/go.mod(1 hunks)types/coin.go(3 hunks)x/bank/go.mod(1 hunks)x/bank/keeper/keeper.go(5 hunks)x/bank/keeper/msg_server.go(2 hunks)x/bank/keeper/send.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/coin/coin.go
🧰 Additional context used
📓 Path-based instructions (4)
x/bank/keeper/msg_server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
types/coin.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/keeper/send.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/keeper/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 GitHub Actions: CodeQL
simapp/v2/go.mod
[warning] go mod tidy -e altered go.mod. Changes need to be checked into version control.
x/bank/go.mod
[warning] go mod tidy -e altered go.mod. Changes need to be checked into version control.
go.mod
[warning] go mod tidy -e altered go.mod. Changes need to be checked into version control.
types/coin.go
[error] 12-12: Missing required module dependency: no required module provides package cosmossdk.io/core/coin
🪛 GitHub Check: dependency-review
types/coin.go
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
[failure] 12-12:
no required module provides package cosmossdk.io/core/coin; to add it:
🪛 GitHub Actions: Dependency Review
types/coin.go
[error] 12-12: Missing required module: no required module provides package cosmossdk.io/core/coin
🪛 GitHub Actions: Tests / Code Coverage
types/coin.go
[error] 12-12: Missing required module dependency: no required module provides package cosmossdk.io/core/coin
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (6)
x/bank/keeper/msg_server.go (1)
60-66: Verify amount conversion safetyThe conversion from sdk.Coins to core.Coins looks correct, but we should ensure that BigInt() never returns nil to prevent potential panics.
✅ Verification successful
Conversion is properly validated and safe
The conversion from sdk.Coins to core.Coins is protected by:
- Prior validation through IsSendEnabledCoins
- Controlled iteration over msg.Amount
- Standard Cosmos SDK protobuf initialization patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for nil amount handling in the codebase rg -A 2 "Amount: \*coin\.Amount\.BigInt\(\)"Length of output: 172
Script:
#!/bin/bash # Check the type definition and validation ast-grep --pattern 'type $_ struct { $$$ Amount $_ $$$ }' # Look for any validation in the msg_server rg -B 5 "Amount\.BigInt\(\)" x/bank/keeper/msg_server.go # Check for similar patterns rg "Amount\.BigInt\(\)"Length of output: 65933
Script:
#!/bin/bash # Check the full context of the MsgServer implementation rg -B 10 -A 10 "func \(k Keeper\) .*\(ctx context\.Context, msg \*" x/bank/keeper/msg_server.go # Look for any validation of msg before this conversion rg -B 10 "Amount\s*=\s*append\(coreAmount" x/bank/keeper/msg_server.goLength of output: 503
Script:
#!/bin/bash # Check the message type definition and any validation rg -B 5 -A 5 "type.*Msg.*struct.*Amount" x/bank/keeper/ rg -B 5 -A 5 "type.*Msg.*struct.*Amount" x/bank/types/Length of output: 114
Script:
#!/bin/bash # Search for the message definition with a broader pattern ast-grep --pattern 'message $_ { $$$ repeated $_ Amount = $_; $$$ }' # Also check for any validation rules rg -B 5 -A 5 "validate.*Amount" x/bank/Length of output: 118
x/bank/keeper/keeper.go (1)
43-45: Ensure consistent coin type usage across interfacesThe change to use core.Coins in these methods is good for consistency. However, ensure all callers are updated to use the new type.
x/bank/go.mod (2)
172-173:⚠️ Potential issueRun
go mod tidy -eto resolve inconsistencies.The pipeline indicates that
go mod tidy -ehas altered the file. Please run the command and commit the changes.Run the following script to verify the module dependencies:
🧰 Tools
🪛 GitHub Actions: CodeQL
[warning] go mod tidy -e altered go.mod. Changes need to be checked into version control.
172-173: Replace directives are consistent across all files.The addition of replace directives for
cosmossdk.io/coreis consistent across all three files, with correct relative paths:
x/bank/go.mod:../../corego.mod:./coresimapp/v2/go.mod:../../coreAlso applies to: 179-179, 286-286
🧰 Tools
🪛 GitHub Actions: CodeQL
[warning] go mod tidy -e altered go.mod. Changes need to be checked into version control.
go.mod (1)
179-179:⚠️ Potential issueRun
go mod tidy -eto resolve inconsistencies.The pipeline indicates that
go mod tidy -ehas altered the file. Please run the command and commit the changes.Run the following script to verify the module dependencies:
🧰 Tools
🪛 GitHub Actions: CodeQL
[warning] go mod tidy -e altered go.mod. Changes need to be checked into version control.
simapp/v2/go.mod (1)
286-286:⚠️ Potential issueRun
go mod tidy -eto resolve inconsistencies.The pipeline indicates that
go mod tidy -ehas altered the file. Please run the command and commit the changes.Run the following script to verify the module dependencies:
🧰 Tools
🪛 GitHub Actions: CodeQL
[warning] go mod tidy -e altered go.mod. Changes need to be checked into version control.
| @@ -2,7 +2,7 @@ package keeper | |||
|
|
|||
| import ( | |||
| "context" | |||
|
|
|||
| core "cosmossdk.io/core/coin" | |||
There was a problem hiding this comment.
Add missing module dependency
The cosmossdk.io/core/coin module is not available in the module requirements.
Add the following to your go.mod file:
+ require cosmossdk.io/core v0.1.0Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
We've discussed in our standup that for a better UX it is best to not add this to core.
This is because the type won't be exactly identical and requires an active migration at many place, without really getting rid of sdk.Coins from the SDK.
The proposal was to add it in math instead
Description
Closes: #XXXX
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 changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
coinpackage for precise cryptocurrency coin representationImprovements
Technical Updates