feat(poa): added minting and burning messages - #1
Conversation
📝 WalkthroughWalkthroughA new CBDC module is integrated into the application, adding central bank digital currency functionality through protobuf message/query definitions, keeper implementation with mint/burn operations, module scaffolding, and wiring into the main app. Additionally, dependency updates and configuration changes are applied. ChangesCBDC Module Integration
Dependency & Configuration Updates
POA Module Test Update
Sequence Diagram(s)sequenceDiagram
actor User
participant TxHandler
participant MsgServer
participant Keeper
participant BankKeeper
participant ModuleAccount
User->>TxHandler: Submit MsgMint(authority, address, amount)
TxHandler->>MsgServer: Mint(context, msg)
MsgServer->>MsgServer: Validate authority
MsgServer->>Keeper: ExecuteMint(ctx, address, amount)
Keeper->>Keeper: Validate denom & amount
Keeper->>Keeper: Decode address
Keeper->>BankKeeper: MintCoins(module, amount)
BankKeeper->>ModuleAccount: Create coins
Keeper->>BankKeeper: SendCoinsFromModuleToAccount(module, address, amount)
BankKeeper->>User: Transfer coins
Keeper->>Keeper: Emit Mint event
Keeper-->>MsgServer: Success
MsgServer-->>TxHandler: MsgMintResponse
sequenceDiagram
actor User
participant TxHandler
participant MsgServer
participant Keeper
participant BankKeeper
participant ModuleAccount
User->>TxHandler: Submit MsgBurn(authority, address, amount)
TxHandler->>MsgServer: Burn(context, msg)
MsgServer->>MsgServer: Validate authority
MsgServer->>Keeper: ExecuteBurn(ctx, address, amount)
Keeper->>Keeper: Validate denom & amount
Keeper->>Keeper: Decode address
Keeper->>BankKeeper: SendCoinsFromAccountToModule(address, module, amount)
BankKeeper->>User: Debit coins
BankKeeper->>ModuleAccount: Credit coins
Keeper->>BankKeeper: BurnCoins(module, amount)
BankKeeper->>ModuleAccount: Destroy coins
Keeper->>Keeper: Emit Burn event
Keeper-->>MsgServer: Success
MsgServer-->>TxHandler: MsgBurnResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (4)
x/poa/types/codec.go (1)
20-31: Optional: consolidateRegisterImplementationscalls.The four separate
RegisterImplementations((*sdk.Msg)(nil), ...)calls can be collapsed into a single variadic call for brevity and consistency with typical Cosmos SDK module codec patterns.♻️ Proposed refactor
- registry.RegisterImplementations((*sdk.Msg)(nil), - &MsgAddValidator{}, - ) - registry.RegisterImplementations((*sdk.Msg)(nil), - &MsgRemoveValidator{}, - ) - registry.RegisterImplementations((*sdk.Msg)(nil), - &MsgMint{}, - ) - registry.RegisterImplementations((*sdk.Msg)(nil), - &MsgBurn{}, - ) + registry.RegisterImplementations((*sdk.Msg)(nil), + &MsgAddValidator{}, + &MsgRemoveValidator{}, + &MsgMint{}, + &MsgBurn{}, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/poa/types/codec.go` around lines 20 - 31, Consolidate the four separate registry.RegisterImplementations calls into one variadic call: locate the uses of registry.RegisterImplementations and replace the repeated calls that register MsgAddValidator, MsgRemoveValidator, MsgMint, and MsgBurn with a single registry.RegisterImplementations((*sdk.Msg)(nil), &MsgAddValidator{}, &MsgRemoveValidator{}, &MsgMint{}, &MsgBurn{}); keep the same order and types so all message implementations remain registered.x/poa/types/events.go (1)
8-13: Duplicate attribute constant values — consolidateAttributeValidatorandAttributeAddress.Both
AttributeValidator(line 8) and the newly addedAttributeAddress(line 12) map to the same string"address". Having two constants with identical values for overlapping semantics (validator address vs. generic address) invites accidental divergence and makes event-attribute parsers ambiguous about which key to consume.Consider either:
- Reusing
AttributeValidatorfrom the mint/burn emitters (if "address" is intentionally the shared key), and droppingAttributeAddress; or- Renaming
AttributeAddressto a distinct value such as"target_address"so mint/burn events are unambiguously distinguishable from validator events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/poa/types/events.go` around lines 8 - 13, AttributeValidator and AttributeAddress both resolve to the string "address", creating ambiguity; update the constants so event attribute keys are unique and unambiguous by either removing AttributeAddress and reusing AttributeValidator in all emitters that expect "address", or renaming AttributeAddress to a distinct key (e.g., "target_address") and update all emitters/parsers that reference AttributeAddress accordingly; look for usages of AttributeValidator and AttributeAddress in emitters/parsers to change references together to avoid mismatches.x/poa/keeper/keeper.go (1)
209-265: Optional: factor out the common mint/burn skeleton.
ExecuteMintandExecuteBurnshare the same shape (validate amount → decode bech32 → two bank calls → emit event with the same two attributes). If you add more asset-movement handlers in the future, consider extracting a small helper that takes the two bank-call closures and the event type. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/poa/keeper/keeper.go` around lines 209 - 265, ExecuteMint and ExecuteBurn share identical structure (validate amount → AccAddressFromBech32 → sdk.NewCoins → two bank operations → emit event) so extract a private helper (e.g., handleAssetAction or executeAssetTransfer) that accepts the address string, amount sdk.Coin, the ctx, the event type (types.EventTypeMint/types.EventTypeBurn) and two function parameters/closures for the bank operations (firstOp, secondOp) which execute the specific k.bk calls (MintCoins/SendCoinsFromModuleToAccount and SendCoinsFromAccountToModule/BurnCoins). Move the common validation, bech32 decode, coins creation, and event emission into the helper and update ExecuteMint and ExecuteBurn to call it with the appropriate closures and event type.x/poa/types/tx_mint.go (1)
1-12: Prefer a generatedtx.pb.goover a hand-maintained encoder.The header comment itself directs future maintainers to run
make proto-geninstead of editing this file, yet the file is committed as a permanent hand-written encoder. That creates two sources of truth — the proto and this Go file — which will diverge the next time someone adds a field toMsgMintinproto/poa/tx.protoor the next timemake proto-genis run (the generatedtx.pb.gowill collide with these symbols). A subtle on-the-wire bug introduced by any future manual edit also won't be caught by tests, since there are no round-trip/marshal tests for this file.Strongly consider either:
- Wiring up
buf generate/protoc-gen-gogoagainstproto/poa/tx.protoand removingtx_mint.go/tx_burn.gofrom version control (regenerated on build), or- At minimum, adding round-trip tests (
Marshal→Unmarshal→ equality) and golden-vector tests against a reference gogo-generated encoder to lock behavior.The encoding logic itself (tag bytes
0x0a/0x12/0x1a, reverse write order, non-nullableAmount) looks correct against standard gogoproto output, but that correctness is fragile without generator tooling or tests to protect it.Also applies to: 53-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/poa/types/tx_mint.go` around lines 1 - 12, This file provides a hand-written proto encoder for MsgMint (symbols: MsgMint, Marshal, Unmarshal, Amount) which risks divergence from the canonical proto; either remove tx_mint.go (and tx_burn.go) and wire up the proto generator (buf/protoc-gen-gogo) to produce tx.pb.go from proto/poa/tx.proto, or keep the manual implementations but add comprehensive tests: round-trip tests that Marshal → Unmarshal and compare equality for MsgMint and golden-vector tests against a gogo-generated encoder to lock behavior and catch regressions; ensure CI runs proto generation or the tests so the single source of truth is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/poa/keeper/keeper.go`:
- Around line 227-233: The event currently emits the raw input address string
(variable `address`) which can differ in formatting; update the mint and burn
event emissions to use the canonical bech32 form by calling the decoded
address's String() method (e.g., replace `address` with `toAddr.String()` in the
mint path and use `fromAddr.String()` in `ExecuteBurn`), and apply the same
change in the other similar blocks noted (around the 256-262 area) so
`types.AttributeAddress` always contains the normalized bech32 address.
- Around line 209-265: Add a denom allowlist guard to Prevent minting/burning of
the staking bond denom: in Keeper.ExecuteMint and Keeper.ExecuteBurn validate
the passed sdk.Coin's Denom against module params (e.g. params.BondDenom) or a
configured allowlist before proceeding (after the IsPositive check and Bech32
address parse); return a clear error (new or existing) if the denom is
disallowed so you never call k.bk.MintCoins, k.bk.SendCoinsFromModuleToAccount,
k.bk.SendCoinsFromAccountToModule, or k.bk.BurnCoins for the forbidden denom.
Ensure the same guard logic is applied symmetrically in both ExecuteMint and
ExecuteBurn and reference params.BondDenom when implementing the rejection.
---
Nitpick comments:
In `@x/poa/keeper/keeper.go`:
- Around line 209-265: ExecuteMint and ExecuteBurn share identical structure
(validate amount → AccAddressFromBech32 → sdk.NewCoins → two bank operations →
emit event) so extract a private helper (e.g., handleAssetAction or
executeAssetTransfer) that accepts the address string, amount sdk.Coin, the ctx,
the event type (types.EventTypeMint/types.EventTypeBurn) and two function
parameters/closures for the bank operations (firstOp, secondOp) which execute
the specific k.bk calls (MintCoins/SendCoinsFromModuleToAccount and
SendCoinsFromAccountToModule/BurnCoins). Move the common validation, bech32
decode, coins creation, and event emission into the helper and update
ExecuteMint and ExecuteBurn to call it with the appropriate closures and event
type.
In `@x/poa/types/codec.go`:
- Around line 20-31: Consolidate the four separate
registry.RegisterImplementations calls into one variadic call: locate the uses
of registry.RegisterImplementations and replace the repeated calls that register
MsgAddValidator, MsgRemoveValidator, MsgMint, and MsgBurn with a single
registry.RegisterImplementations((*sdk.Msg)(nil), &MsgAddValidator{},
&MsgRemoveValidator{}, &MsgMint{}, &MsgBurn{}); keep the same order and types so
all message implementations remain registered.
In `@x/poa/types/events.go`:
- Around line 8-13: AttributeValidator and AttributeAddress both resolve to the
string "address", creating ambiguity; update the constants so event attribute
keys are unique and unambiguous by either removing AttributeAddress and reusing
AttributeValidator in all emitters that expect "address", or renaming
AttributeAddress to a distinct key (e.g., "target_address") and update all
emitters/parsers that reference AttributeAddress accordingly; look for usages of
AttributeValidator and AttributeAddress in emitters/parsers to change references
together to avoid mismatches.
In `@x/poa/types/tx_mint.go`:
- Around line 1-12: This file provides a hand-written proto encoder for MsgMint
(symbols: MsgMint, Marshal, Unmarshal, Amount) which risks divergence from the
canonical proto; either remove tx_mint.go (and tx_burn.go) and wire up the proto
generator (buf/protoc-gen-gogo) to produce tx.pb.go from proto/poa/tx.proto, or
keep the manual implementations but add comprehensive tests: round-trip tests
that Marshal → Unmarshal and compare equality for MsgMint and golden-vector
tests against a gogo-generated encoder to lock behavior and catch regressions;
ensure CI runs proto generation or the tests so the single source of truth is
enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2373c7a2-a544-4ad3-ba96-3cf08a3b01ec
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumx/poa/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
go.modproto/poa/tx.protox/poa/keeper/keeper.gox/poa/keeper/keeper_test.gox/poa/keeper/msg_server_burn.gox/poa/keeper/msg_server_burn_test.gox/poa/keeper/msg_server_mint.gox/poa/keeper/msg_server_mint_test.gox/poa/types/codec.gox/poa/types/errors.gox/poa/types/events.gox/poa/types/message_burn.gox/poa/types/message_mint.gox/poa/types/tx_burn.gox/poa/types/tx_mint.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker / Build
🔇 Additional comments (7)
x/poa/keeper/msg_server_burn.go (1)
13-24: LGTM.Authority check, context unwrap, and delegation to
ExecuteBurnmatch the existing message server pattern in this module. Amount validation (positive check) and bech32 decoding are correctly pushed intoExecuteBurn(x/poa/keeper/keeper.go), keeping the handler thin.x/poa/keeper/msg_server_burn_test.go (1)
17-18: Same verification as inmsg_server_mint_test.go.
TestMsgServer_Burn's "should pass" case likewise has no bank expectations forSendCoinsFromAccountToModule/BurnCoins. Whatever resolution applies to the mint test applies here — please ensurepoaKeeperTestSetupcovers both success paths.Also applies to: 48-53
x/poa/types/tx_burn.go (1)
1-364: Same concerns astx_mint.go.This file is a near-identical copy of
tx_mint.gowith only the type name changed. The recommendation to replace both hand-written encoders with generatedtx.pb.goapplies equally here — keeping both files in sync manually will be error-prone.x/poa/keeper/msg_server_mint.go (1)
13-24: LGTM.Handler follows the standard Cosmos-SDK authority-gated message pattern: signer check against
k.authority, SDK context unwrap, delegation to the keeper method, and empty response on success. Consistent with howMsgAddValidator/MsgRemoveValidatorare wired.One small note: the authority comparison is a raw string equality check. Since
k.authorityis validated as bech32 inNewKeeperbutmsg.Authorityis not normalized here, two bech32-equivalent strings differing in case would be rejected. This matches the upstream gov/authz convention, so it's fine — just be aware that all callers (including tests and tooling) must use the canonical form stored ink.authority.proto/poa/tx.proto (1)
22-25: LGTM.The new
Mint/BurnRPCs and their message definitions are structurally consistent with the existingAddValidator/RemoveValidatormessages:authorityas the designated signer,AddressStringscalar constraints on both address fields, and non-nullableCoinforamount. Import ofcosmos/base/v1beta1/coin.protois correctly added.Also applies to: 55-77
x/poa/types/tx_mint.go (1)
62-110: All helper symbols and errors confirmed to exist in x/poa/types.The file correctly references
encodeVarintTx,sovTx,skipTx,ErrIntOverflowTx, andErrInvalidLengthTx, which are all defined in the generatedtx.pb.gofile. No compilation issues will occur.x/poa/keeper/msg_server_mint_test.go (1)
17-18: No action needed—bank mocks are properly configured.The
poaKeeperTestSetuphelper pre-configures the bank keeper mock with.AnyTimes()for bothMintCoinsandSendCoinsFromModuleToAccount(keeper_test.go lines 44-45). These expectations will silently accept any calls made by the "should pass" test case and returnnil, so the test will succeed without additional per-test-case setup.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
x/poa/keeper/keeper.go (1)
237-243:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmit canonical Bech32 address in mint/burn events.
types.AttributeAddressstill uses raw input. On Line 240 and Line 272, emit decoded canonical values (toAddr.String()/fromAddr.String()) to keep indexer queries consistent.🔧 Proposed fix
ctx.EventManager().EmitEvent( sdk.NewEvent( types.EventTypeMint, - sdk.NewAttribute(types.AttributeAddress, address), + sdk.NewAttribute(types.AttributeAddress, toAddr.String()), sdk.NewAttribute(types.AttributeAmount, amount.String()), ), ) @@ ctx.EventManager().EmitEvent( sdk.NewEvent( types.EventTypeBurn, - sdk.NewAttribute(types.AttributeAddress, address), + sdk.NewAttribute(types.AttributeAddress, fromAddr.String()), sdk.NewAttribute(types.AttributeAmount, amount.String()), ), )Also applies to: 269-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/poa/keeper/keeper.go` around lines 237 - 243, The mint/burn event is emitting the raw input `address` string rather than the canonical Bech32 address; update the event attributes in the `ctx.EventManager().EmitEvent` calls (the ones creating `sdk.NewEvent` with `types.EventTypeMint` and the corresponding burn event) to use the decoded address object's `String()` (e.g., `toAddr.String()` for mint and `fromAddr.String()` for burn) for `types.AttributeAddress`, leaving `amount.String()` unchanged so indexers receive canonical Bech32 addresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@x/poa/keeper/keeper.go`:
- Around line 237-243: The mint/burn event is emitting the raw input `address`
string rather than the canonical Bech32 address; update the event attributes in
the `ctx.EventManager().EmitEvent` calls (the ones creating `sdk.NewEvent` with
`types.EventTypeMint` and the corresponding burn event) to use the decoded
address object's `String()` (e.g., `toAddr.String()` for mint and
`fromAddr.String()` for burn) for `types.AttributeAddress`, leaving
`amount.String()` unchanged so indexers receive canonical Bech32 addresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79c2c6af-690a-4fc4-871f-dac08f80323a
⛔ Files ignored due to path filters (1)
x/poa/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
app/app.gox/poa/keeper/common_test.gox/poa/keeper/keeper.gox/poa/keeper/msg_server_burn_test.gox/poa/keeper/msg_server_mint_test.gox/poa/types/errors.go
✅ Files skipped from review due to trivial changes (2)
- x/poa/keeper/common_test.go
- x/poa/keeper/msg_server_burn_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/poa/types/errors.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker / Build
🔇 Additional comments (4)
x/poa/keeper/keeper.go (1)
64-66: Good fail-fast validation for configured CBDC denom.Validating
cbdcDenomduring keeper construction and storing it in the keeper reduces runtime misconfiguration risk.Also applies to: 75-75
app/app.go (1)
465-473: Keeper wiring update looks correct.Passing
BaseDenomintopoakeeper.NewKeepercorrectly aligns app initialization with the new mint/burn denom restriction.x/poa/keeper/msg_server_mint_test.go (2)
16-80: Solid message-server mint test matrix.Good coverage of authority, address, amount, denom, and success paths for
MsgMint.
82-155: Keeper mint tests cover core failure propagation well.The table-driven cases correctly exercise validation and mocked bank keeper failures for
ExecuteMint.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
x/cbdc/types/codec.go (1)
23-25: ⚡ Quick winRemove empty
govtypes.Contentregistration — dead scaffolding code
RegisterImplementations((*govtypes.Content)(nil))with no concrete types is a no-op left over from Ignite CLI scaffolding. It also pulls ingovtypes "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"unnecessarily. Remove both the call and the import unless a governance proposal type is actually planned.♻️ Proposed cleanup
import ( "github.com/cosmos/cosmos-sdk/codec" cdctypes "github.com/cosmos/cosmos-sdk/codec/types" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/msgservice" - govtypes "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" ) func RegisterInterfaces(registry cdctypes.InterfaceRegistry) { registry.RegisterImplementations((*sdk.Msg)(nil), &MsgMint{}, + &MsgBurn{}, ) - registry.RegisterImplementations((*sdk.Msg)(nil), - &MsgBurn{}, - ) - registry.RegisterImplementations( - (*govtypes.Content)(nil), - ) msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/cbdc/types/codec.go` around lines 23 - 25, Remove the no-op governance registration and unused import: delete the call to registry.RegisterImplementations((*govtypes.Content)(nil)) in codec.go and remove the govtypes import (github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1); ensure no other code depends on govtypes.Content before removing and run goimports/go vet to clean up remaining unused imports.x/cbdc/types/params.go (1)
1-32: 🏗️ Heavy liftRemove deprecated
x/paramsscaffolding from this module
x/paramsis deprecated as of Cosmos SDK v0.53 and will be removed in v0.54. This module currently uses v0.53.6-xrplevm.1, so this scaffolding is building on a module that is about to disappear.Since
Paramsis empty and theParamSetPairs()returns an empty set with no actual parameter handling (theGetParams()method always returnsNewParams()regardless of stored state), the entirex/paramsintegration intypes/params.goandkeeper/params.gois dead code.The recommended approach per ADR-046 is to remove this scaffolding and implement parameters via protobuf-based
MsgUpdateParamsmessages when parameters are actually needed. Other modules in this codebase (e.g., x/poa) already follow this pattern. If this module needs governance-controlled parameter updates in the future, add aMsgUpdateParamshandler totx.protothat stores parameters directly in the module's KV store.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/cbdc/types/params.go` around lines 1 - 32, Remove the deprecated x/params scaffolding by deleting the empty Params type usages and related functions: ParamKeyTable, NewParams, DefaultParams, ParamSetPairs, Validate and the ParamSet interface assertion for Params in types/params.go, and remove the corresponding keeper/params.go integration that only returns NewParams/GetParams; instead, leave module without x/params hooks and plan to implement protobuf-based MsgUpdateParams and direct KV storage if parameters are needed later per ADR-046. Ensure any references to ParamKeyTable or ParamSetPairs in the module/keeper are also removed or replaced so there are no unused imports or dead code.x/cbdc/keeper/common_test.go (1)
82-91: 💤 Low valueOptional: use
gomock.Any()for the context argument in bank expectations.The expectations match against the exact
ctxvalue. This works today becauseExecuteMint/ExecuteBurndon't wrap the context, but any future change (gas metering, cached store, event-manager swap) could produce a non-equal context and fail these matchers. Since the assertions already useAnyTimes()withgomock.Any()for the other arguments, matching the context the same way is more robust and equally precise.♻️ Proposed change
bankExpectations := func(ctx sdk.Context, bankKeeper *testutil.MockBankKeeper) { - bankKeeper.EXPECT().MintCoins(ctx, gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - bankKeeper.EXPECT().SendCoinsFromModuleToAccount(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - bankKeeper.EXPECT().BurnCoins(ctx, gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - bankKeeper.EXPECT().SendCoinsFromAccountToModule(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + bankKeeper.EXPECT().MintCoins(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + bankKeeper.EXPECT().SendCoinsFromModuleToAccount(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + bankKeeper.EXPECT().BurnCoins(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() }If you want to keep the function signature for callers that pass
ctx, simply ignore the parameter via_inside the closure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/cbdc/keeper/common_test.go` around lines 82 - 91, The bank expectations closure in cbdcKeeperTestSetup captures the concrete ctx and uses it in EXPECT() calls which can be brittle; change the closure (bankExpectations) to ignore the passed ctx (use _ or don't rely on it) and use gomock.Any() for the context argument in all EXPECT() calls to MintCoins, SendCoinsFromModuleToAccount, BurnCoins, and SendCoinsFromAccountToModule so the matchers remain robust while keeping the existing AnyTimes() behavior; keep calling setupCbdcKeeper unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/app.go`:
- Around line 479-485: initParamsKeeper never registers cbdctypes.ModuleName, so
app.GetSubspace(cbdctypes.ModuleName) returns an empty Subspace and causes
nil-store panics when CbdcKeeper uses SetParams/GetParams; fix by adding
paramsKeeper.Subspace(cbdctypes.ModuleName) in the initParamsKeeper registration
block (alongside the other Ethermint/EVT subspace registrations) so the params
keeper's internal spaces map contains cbdctypes.ModuleName before
cbdckeeper.NewKeeper/app.GetSubspace is used.
- Line 484: Add explicit documentation next to the CBDC keeper initialization
that confirms the native token (BaseDenom, i.e. "acbdc") is the chain's CBDC and
clarify the implications: governance via the CBDC keeper/bank keeper can
mint/burn the native denom (not a separate "ucbdc" denom), this gives gov direct
control over native supply and is irreversible without a chain upgrade; update
the comment surrounding the CBDC keeper construction (reference symbols:
BaseDenom, CBDC keeper, bank keeper) and any public godoc for the keeper so
future readers clearly understand this design choice and its governance/supply
implications.
In `@proto/cbdc/params.proto`:
- Around line 1-7: Buf lint fails because the proto package "cbdc" in
proto/cbdc/ does not match the buf root; fix by either (preferred) making the
proto/ directory the buf root (add a buf.yaml at repo root with root: "proto" or
place buf.yaml in proto/ and set root accordingly) so package cbdc in
proto/cbdc/ is valid, or alternatively change the proto package name in
proto/cbdc/params.proto (the `package cbdc;` and related `option go_package =
"github.com/peersyst/cbdc-node/x/cbdc/types";`) to a fully-qualified name that
matches the repository path relative to the current buf root; implement one of
these fixes and re-run buf lint.
In `@proto/cbdc/tx.proto`:
- Around line 4-8: The proto file imports "amino/amino.proto" but the buf deps
lack the amino repo; add the dependency entry "buf.build/amino/amino" to the
deps array in proto/buf.yaml so the import resolves, then run "buf dep update"
in the proto directory and commit the resulting updated proto/buf.lock.
In `@x/cbdc/keeper/genesis.go`:
- Around line 13-19: GetParams currently returns types.NewParams() and ignores
the param store, causing queries/exports to show defaults; update
Keeper.GetParams to read the stored params by declaring a params variable of
type types.Params and calling k.paramstore.GetParamSet(ctx, ¶ms) (or
paramstore.GetParamSet depending on receiver) and then return params so it
mirrors SetParams which uses paramstore.SetParamSet; ensure you reference the
GetParams and SetParams methods and the param store GetParamSet call when making
the change.
In `@x/cbdc/keeper/params.go`:
- Around line 9-11: GetParams currently ignores the paramstore and returns
types.NewParams(), so it never returns persisted values; change GetParams to
read the stored params from k.paramstore into a local types.Params variable
(mirror how SetParams persists values) using the paramstore's Get/Unmarshal
method (e.g., k.paramstore.Get or equivalent) with the correct key/paramset and
then return that populated params struct instead of types.NewParams(); ensure
you reference the existing GetParams, SetParams, k.paramstore, and types.Params/
types.NewParams symbols when making the change.
In `@x/cbdc/module.go`:
- Around line 97-100: In RegisterServices (AppModule.RegisterServices) you're
registering the query server with the raw Keeper instead of the required Querier
wrapper; replace passing am.keeper to types.RegisterQueryServer with the wrapped
instance created by keeper.NewQuerier(am.keeper) (or NewQuerier if
package-qualified), so the registered implementation matches the asserted type
Querier that implements types.QueryServer.
---
Nitpick comments:
In `@x/cbdc/keeper/common_test.go`:
- Around line 82-91: The bank expectations closure in cbdcKeeperTestSetup
captures the concrete ctx and uses it in EXPECT() calls which can be brittle;
change the closure (bankExpectations) to ignore the passed ctx (use _ or don't
rely on it) and use gomock.Any() for the context argument in all EXPECT() calls
to MintCoins, SendCoinsFromModuleToAccount, BurnCoins, and
SendCoinsFromAccountToModule so the matchers remain robust while keeping the
existing AnyTimes() behavior; keep calling setupCbdcKeeper unchanged.
In `@x/cbdc/types/codec.go`:
- Around line 23-25: Remove the no-op governance registration and unused import:
delete the call to registry.RegisterImplementations((*govtypes.Content)(nil)) in
codec.go and remove the govtypes import
(github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1); ensure no other code depends
on govtypes.Content before removing and run goimports/go vet to clean up
remaining unused imports.
In `@x/cbdc/types/params.go`:
- Around line 1-32: Remove the deprecated x/params scaffolding by deleting the
empty Params type usages and related functions: ParamKeyTable, NewParams,
DefaultParams, ParamSetPairs, Validate and the ParamSet interface assertion for
Params in types/params.go, and remove the corresponding keeper/params.go
integration that only returns NewParams/GetParams; instead, leave module without
x/params hooks and plan to implement protobuf-based MsgUpdateParams and direct
KV storage if parameters are needed later per ADR-046. Ensure any references to
ParamKeyTable or ParamSetPairs in the module/keeper are also removed or replaced
so there are no unused imports or dead code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ededc61-53c7-4798-8bcb-446a265b54fe
⛔ Files ignored due to path filters (5)
x/cbdc/types/genesis.pb.gois excluded by!**/*.pb.gox/cbdc/types/params.pb.gois excluded by!**/*.pb.gox/cbdc/types/query.pb.gois excluded by!**/*.pb.gox/cbdc/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/cbdc/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (30)
app/app.golocal-node.shproto/cbdc/genesis.protoproto/cbdc/params.protoproto/cbdc/query.protoproto/cbdc/tx.protoscripts/mockgen.shx/cbdc/keeper/common_test.gox/cbdc/keeper/genesis.gox/cbdc/keeper/keeper.gox/cbdc/keeper/msg_server.gox/cbdc/keeper/msg_server_burn.gox/cbdc/keeper/msg_server_burn_test.gox/cbdc/keeper/msg_server_mint.gox/cbdc/keeper/msg_server_mint_test.gox/cbdc/keeper/params.gox/cbdc/keeper/query.gox/cbdc/keeper/query_params.gox/cbdc/module.gox/cbdc/module_simulation.gox/cbdc/testutil/expected_keepers_mock.gox/cbdc/types/codec.gox/cbdc/types/errors.gox/cbdc/types/events.gox/cbdc/types/expected_keepers.gox/cbdc/types/genesis.gox/cbdc/types/keys.gox/cbdc/types/message_burn.gox/cbdc/types/message_mint.gox/cbdc/types/params.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker / Build
🧰 Additional context used
🪛 Buf (1.69.0)
proto/cbdc/params.proto
[error] 2-2: Files with package "cbdc" must be within a directory "cbdc" relative to root but were in directory "proto/cbdc".
(PACKAGE_DIRECTORY_MATCH)
proto/cbdc/query.proto
[error] 4-4: imported file does not exist
(COMPILE)
[error] 5-5: imported file does not exist
(COMPILE)
[error] 6-6: imported file does not exist
(COMPILE)
[error] 14-14: cannot find google.api.http in this scope
(COMPILE)
[error] 24-24: cannot find Params in this scope
(COMPILE)
[error] 24-24: cannot find gogoproto.nullable in this scope
(COMPILE)
proto/cbdc/tx.proto
[error] 4-4: imported file does not exist
(COMPILE)
[error] 5-5: imported file does not exist
(COMPILE)
[error] 6-6: imported file does not exist
(COMPILE)
[error] 7-7: imported file does not exist
(COMPILE)
[error] 8-8: imported file does not exist
(COMPILE)
[error] 14-14: cannot find cosmos.msg.v1.service in this scope
(COMPILE)
[error] 24-24: cannot find cosmos.msg.v1.signer in this scope
(COMPILE)
[error] 26-26: cannot find cosmos_proto.scalar in this scope
(COMPILE)
[error] 27-27: cannot find cosmos_proto.scalar in this scope
(COMPILE)
[error] 28-28: cannot find cosmos.base.v1beta1.Coin in this scope
(COMPILE)
[error] 29-29: cannot find gogoproto.nullable in this scope
(COMPILE)
[error] 29-29: cannot find amino.dont_omitempty in this scope
(COMPILE)
[error] 36-36: cannot find cosmos.msg.v1.signer in this scope
(COMPILE)
[error] 38-38: cannot find cosmos_proto.scalar in this scope
(COMPILE)
[error] 39-39: cannot find cosmos_proto.scalar in this scope
(COMPILE)
[error] 40-40: cannot find cosmos.base.v1beta1.Coin in this scope
(COMPILE)
[error] 41-41: cannot find gogoproto.nullable in this scope
(COMPILE)
[error] 41-41: cannot find amino.dont_omitempty in this scope
(COMPILE)
proto/cbdc/genesis.proto
[error] 4-4: imported file does not exist
(COMPILE)
[error] 5-5: imported file does not exist
(COMPILE)
[error] 10-10: cannot find Params in this scope
(COMPILE)
[error] 10-10: cannot find gogoproto.nullable in this scope
(COMPILE)
🔇 Additional comments (23)
local-node.sh (2)
50-50: LGTM —min_gas_pricegenesis patch looks correct.The 18-decimal zero string matches the
sdk.Decserialization format expected by the feemarket module, consistent with the surroundingno_base_fee=trueandbase_fee=0settings.
67-67: ⚡ Quick winVerify that zero-amount fees work with this Cosmos SDK version.
When
BASEFEE=0, the expanded--fees 0axrpmay be rejected by SDK parsing or ante-handler validation depending on the version and chain configuration. Either test the script with--fees 0axrp --dry-runto confirm, use a documented positive fee value, or omit the flag if genesis allows.Additionally, quote the variable expansion to guard against breakage if
BASEFEEis unset or empty:🛡️ Defensive fix
-bin/cbdcd --home "$HOMEDIR" genesis gentx alice 1000000apoa --fees ${BASEFEE}axrp --gas 1000000 --keyring-backend "$KEYRING" --chain-id "$CHAINID" +bin/cbdcd --home "$HOMEDIR" genesis gentx alice 1000000apoa --fees "${BASEFEE}axrp" --gas 1000000 --keyring-backend "$KEYRING" --chain-id "$CHAINID"x/cbdc/types/events.go (1)
1-8: LGTM!Clean, correctly scoped constants. No issues.
x/cbdc/types/message_burn.go (1)
1-15: LGTM!Compile-time guard and constructor are both correct and idiomatic.
x/cbdc/types/message_mint.go (1)
1-15: LGTM!Mirrors
message_burn.gocorrectly; no issues.x/cbdc/types/expected_keepers.go (1)
1-21: LGTM!Both interfaces are correctly defined with
context.Context(appropriate for SDK v0.47+), and the method signatures match the standardx/bankkeeper interface.scripts/mockgen.sh (1)
7-8: LGTM!Both the reordering and the new CBDC mock generation line follow the established pattern and correctly align with the new
x/cbdc/testutil/expected_keepers_mock.gofile.x/cbdc/testutil/expected_keepers_mock.go (1)
1-143: LGTM — standard gomock v1 generated output.All mock implementations, recorder types, and
RecordCallWithMethodTypecalls follow the canonical mockgen v1 pattern and match the interfaces they're sourced from.x/cbdc/keeper/query.go (1)
7-7: LGTM — standard compile-time interface assertion.x/cbdc/keeper/query_params.go (1)
12-19: LGTM — standard Cosmos SDK gRPC query handler.Nil-request guard, context unwrapping, and params retrieval all follow the established SDK pattern correctly.
x/cbdc/keeper/msg_server_mint.go (1)
13-24: LGTM — clean, idiomatic Cosmos SDK message handler.Authority check, context unwrapping, and delegation to
ExecuteMintall follow the established pattern correctly.x/cbdc/types/keys.go (1)
1-19: LGTM — standard Cosmos SDK module key definitions.x/cbdc/keeper/msg_server_burn_test.go (1)
55-60: No action required:cbdcKeeperTestSetupcorrectly pre-registers bank mock expectations.The
cbdcKeeperTestSetupfunction incommon_test.goproperly registers mock expectations for all bank methods needed byExecuteBurn:SendCoinsFromAccountToModuleandBurnCoinsare both set up with.AnyTimes(). The "should pass" test case will not fail or panic due to unmet mock expectations. No refactoring is necessary.proto/cbdc/genesis.proto (1)
1-10: LGTM. The Buf "imported file does not exist" hints look like dependency-path resolution noise rather than real problems with this file (gogoproto, google/api, and the siblingcbdc/params.protoare standard imports for a Cosmos SDK module).proto/cbdc/query.proto (1)
1-25: LGTM. Standard Cosmos SDK Query/Params service; the Buf "imported file does not exist" hints look like dependency-path resolution noise.x/cbdc/keeper/msg_server_burn.go (1)
13-24: LGTM.Authority check, context unwrap, and delegation to
ExecuteBurnmirror the existingMinthandler and use the standard gov error wrapping pattern.x/cbdc/types/genesis.go (1)
1-17: LGTM. Standard genesis scaffolding;Validatecorrectly delegates toParams.Validate.x/cbdc/types/errors.go (1)
1-13: LGTM. Sentinel errors are registered with stable, distinct codes and clear messages.x/cbdc/keeper/msg_server.go (1)
1-17: LGTM!Standard MsgServer wiring; the compile-time assertion plus pointer-returning constructor both satisfy
types.MsgServersince handler methods (inmsg_server_mint.go/msg_server_burn.go) use value receivers.x/cbdc/module_simulation.go (1)
23-42: LGTM!Standard simulation scaffold with placeholder weighted ops and decoders; safe to extend later when simulation coverage is needed.
x/cbdc/keeper/msg_server_mint_test.go (1)
16-155: LGTM!Solid table-driven coverage for both the MsgServer wrapper (authority + delegation) and
ExecuteMint(validation + bank interactions), with explicit assertions on expected error contents and gomock expectations on success paths.x/cbdc/keeper/keeper.go (1)
70-132: LGTM onExecuteMint/ExecuteBurn.Validation order (denom → positive → bech32) is consistent across both methods, bank ops are sequenced correctly (mint→send / send→burn), and events carry both address and amount. Note that authority checks live one level up in the MsgServer handlers, which is the standard Cosmos pattern — keep this in mind if these methods are ever exposed to other in-module callers.
x/cbdc/module.go (1)
104-112: LGTM onInitGenesis.Order is correct: unmarshal → keeper-level genesis init → ensure module account exists. The
GetModuleAccountcall also handles first-time creation for the cbdc module account, which is required forMintCoins/SendCoinsFromModuleToAccountto work later.
JordiParraCrespo
left a comment
JordiParraCrespo
left a comment
There was a problem hiding this comment.
Reviewed the cbdc module against x/erc20 (cosmos/evm) with a focus on mint/burn permissions, supply, and the upgrade path for the privileged address. 41 inline comments grouped roughly as follows:
Critical
app/app.go:1173— cbdc paramstore subspace is never registered ininitParamsKeeper.app.GetSubspace(cbdctypes.ModuleName)silently returns a zero-value Subspace;SetParams/GetParamswill panic or no-op at runtime. Currently masked byParams{}being empty; explodes the momentOwnerlands.x/cbdc/keeper/params.go:10—GetParamsignores the paramstore and returnstypes.NewParams(). Same blast radius as above once params have any field.
Permission / shape (main concern)
keeper.go:28— mint/burn authority is constructor-only, not in chain state. Move it toParams.Owner, rotatable viaMsgUpdateParamssigned by the gov module authority. Mirrorx/erc20's split between the gov-controlled authority and the rotatable per-token owner.proto/cbdc/tx.proto:13and:23— addMsgUpdateParams; renameMsgMint.authority/MsgBurn.authority→ownerto disambiguate fromMsgUpdateParams.authority.types/params.go:16—Paramsneedsowner(required, validated bech32, rejected if empty in genesis) and a gov-controlledpausedflag so gov can stop mint/burn independently of the owner during incidents.
Defensive checks aligned with x/erc20
keeper.go:87— missingBlockedAddrcheck on mint receiver.keeper.go:84— missingIsSendEnabledCoincheck.keeper.go:70— factor pre-flight checks into aMintingAllowed/BurningAllowedhelper.keeper.go:70/:102— makeExecuteMint/ExecuteBurnpackage-private or take a signer arg; defense in depth so the owner check can't be bypassed by other callers in the binary.keeper.go:95— emitsdk.AttributeKeySenderso the audit trail records who authorized each mint/burn.
Queries
proto/cbdc/query.proto:11— add convenienceOwner()andPaused()queries for monitoring clients (mirrorsx/erc20.OwnerAddress).
Cleanup / structure
keeper.go:16andquery.go:7— competingvar _ types.QueryServer = ...assertions;Querieris dead code,module.go:99registersKeeperdirectly.module.go:76— unusedbkfield onAppModule.module.go:102— emptyRegisterInvariants; at minimum register a "module account exists" / "owner is valid bech32" invariant.module.go:119— bumpConsensusVersionand add a v1→v2 migration when the params change ships.app.go:753— cbdc inSetOrderEndBlockersbut noEndBlockmethod.types/codec.go:23— deadgovtypes.Contentregistration with no implementations.types/errors.go— borrowgov.ErrInvalidSignerinstead of defining module-local errors.types/events.go— generic"mint"/"burn"event types, should be namespaced.
Tests / sim
common_test.go:67,module_simulation.go:26— both will break the momentParams.Ownerbecomes required; flagged so the fix lands in the same PR.msg_server_mint_test.go— missing negative-amount case.
Other
x/poa/keeper/keeper_test.go—BurnCoinsmock loosened fromBondedPoolNametogomock.Any(); please confirm whether this hides a real change in burn target.
Requesting changes — the paramstore-subspace bug alone is a release blocker, and the owner-in-state + MsgUpdateParams work is the structural change this PR should land before the module ships.
2 participants
feat(poa): added minting and burning messages
Motivation 💡
This PR aims to add minting and burning capabilities to the poa module
Changes 🛠
Summary by CodeRabbit
New Features
Chores
Tests