fix: limit gas amount for balance query #412
Conversation
📝 WalkthroughWalkthroughThis update introduces support for dispatchable fungible assets within the Move bank keeper, including new logic for balance and supply queries that handle dispatchable assets with gas metering. It adds a new Move module for a test dispatchable token and an invalid dispatchable token, associated tests, helper methods, and constants for dispatchable asset management. Refactoring also consolidates base64 decoding logic for upgrade modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test_DispatchableToken
participant MoveBank as MoveBankKeeper
participant MoveVM as Move VM
participant Dispatchable as Dispatchable Token Module
Test->>MoveVM: Publish upgrade modules
Test->>MoveVM: Publish DispatchableToken module (deployer)
Test->>MoveVM: Execute initialize entry function
Test->>MoveBank: GetSupplyWithMetadata (dispatchable token)
MoveBank->>MoveVM: Check HasDispatchSupplyStore
alt Dispatchable supply store exists
MoveBank->>MoveVM: Execute derived_supply view (gas-limited)
MoveVM->>MoveBank: Return supply (multiplied by 10)
else
MoveBank->>MoveVM: Query standard supply
end
Test->>MoveVM: Execute mint entry function
Test->>MoveBank: GetBalance (dispatchable token)
MoveBank->>MoveVM: Check HasDispatchFunctionStore
alt Dispatchable function store exists
MoveBank->>MoveVM: Execute derived_balance view (gas-limited)
MoveVM->>MoveBank: Return balance (multiplied by 10)
else
MoveBank->>MoveVM: Query standard balance
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.Scanned FilesNone |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
x/move/keeper/bank_test.go (1)
55-61:⚠️ Potential issue
for i := range tokenNumdoes not compile
tokenNumis anint, not an iterable collection.
Use an indexed loop:-for i := range tokenNum { +for i := 0; i < tokenNum; i++ {The same fix is required in every identical loop in this file (e.g. line 111).
🧹 Nitpick comments (2)
x/move/keeper/bank_test.go (1)
332-344: Temporary upgrade-module publishing block risks test flakinessThe TODO indicates this block should vanish after a MoveVM bump.
Consider guarding it behind a build tag or helper to prevent accidental removal from breaking CI when the upgrade lands.x/move/keeper/contracts/sources/DispatchableToken.move (1)
19-64: Consider making the initial supply configurable.The initialization function is well-structured, but the initial supply is hardcoded to a very large value (1,000,000,000,000,000,000). Consider making this configurable or documenting why this specific value was chosen.
Apply this diff to make the initial supply configurable:
-public entry fun initialize(deployer: &signer) { +public entry fun initialize(deployer: &signer, initial_supply: u128) { let constructor_ref = &object::create_named_object(deployer, b"test_token"); primary_fungible_store::create_primary_store_enabled_fungible_asset( constructor_ref, - option::some(1000000000000000000), + option::some(initial_supply), string::utf8(b"Test Token Name"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumx/move/keeper/contracts/Move.tomlis excluded by!**/*.toml
📒 Files selected for processing (8)
app/upgrades/v1_1_1/modules.go(2 hunks)app/upgrades/v1_1_1/upgrade.go(1 hunks)x/move/keeper/bank.go(3 hunks)x/move/keeper/bank_test.go(2 hunks)x/move/keeper/common_test.go(2 hunks)x/move/keeper/contracts/sources/DispatchableToken.move(1 hunks)x/move/keeper/fungible_asset.go(1 hunks)x/move/types/connector.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
x/move/keeper/fungible_asset.go (3)
x/move/keeper/bank.go (1)
MoveBankKeeper(25-27)x/bank/types/expected_keeper.go (1)
MoveBankKeeper(13-34)x/move/types/connector.go (3)
MoveModuleNameFungibleAsset(23-23)ResourceNameDispatchFunctionStore(97-97)ResourceNameDispatchSupply(98-98)
app/upgrades/v1_1_1/upgrade.go (1)
app/upgrades/v1_1_1/modules.go (1)
GetModuleBytes(22-33)
x/move/keeper/common_test.go (1)
x/gov/keeper/common_test.go (1)
ReadMoveFile(506-513)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
x/move/keeper/common_test.go (1)
499-499: LGTM! Consistent pattern for adding test module.The addition of
dispatchableTokenModulefollows the established pattern used by other module variables in this file. The variable naming is descriptive and the initialization in theinit()function is consistent with existing modules.Also applies to: 509-509
app/upgrades/v1_1_1/modules.go (2)
3-3: LGTM! Necessary import for base64 decoding.The addition of the
encoding/base64import is appropriate for the newGetModuleBytes()function.
22-33: Excellent refactoring with proper error handling.The
GetModuleBytes()function effectively centralizes the base64 decoding logic with:
- Clear documentation and function signature
- Proper error handling that returns early on decode failures
- Clean, readable implementation
- Good separation of concerns by extracting this logic from the upgrade handler
app/upgrades/v1_1_1/upgrade.go (1)
28-31: Good refactoring to use centralized decoding function.The refactoring successfully delegates base64 decoding to the
GetModuleBytes()helper function while maintaining proper error handling. This improves code maintainability and reduces duplication.x/move/keeper/fungible_asset.go (2)
152-160: Well-implemented method following established patterns.The
HasDispatchFunctionStoremethod correctly:
- Follows the naming convention and signature pattern of other
Has*methods- Uses appropriate
StructTagconfiguration for theDispatchFunctionStoreresource- Maintains consistency with existing code structure
162-170: Consistent implementation for dispatch supply checking.The
HasDispatchSupplyStoremethod properly:
- Maintains consistent method signature and implementation pattern
- Uses the correct
ResourceNameDispatchSupplyconstant in theStructTag- Follows the same error handling approach as other resource checking methods
x/move/types/connector.go (1)
96-99: Resource name constant typo?
ResourceNameDispatchSupplyis set to"DeriveSupply"while the dispatch function constant uses"derived_supply".
If the on-chain resource is actuallyDeriveSupplythis is fine, but the mismatch in verb tense (DerivevsDerived) could lead to hard-to-trace bugs. Please double-check the Move module before release.x/move/keeper/contracts/sources/DispatchableToken.move (4)
1-12: Module imports and dependencies look appropriate.The module correctly imports necessary dependencies for fungible asset operations and dispatchable functionality. The namespace organization is clean and follows Move conventions.
13-17: ManagingRefs struct is well-designed.The struct appropriately stores the three key references needed for fungible asset management. The
keyability is correctly applied for storing under an account.
66-69: Derived balance logic is correct and efficient.The derived balance function correctly implements the 10x multiplication logic. The implementation is simple and efficient.
81-85: 🛠️ Refactor suggestionAdd access control to the mint function.
The mint function lacks access control, allowing anyone to mint tokens if they have access to the deployer's account. Consider adding proper authorization checks.
Apply this diff to add basic access control:
public entry fun mint(account: &signer, to: address, amount: u64) acquires ManagingRefs { + // Only the deployer can mint tokens + assert!(exists<ManagingRefs>(signer::address_of(account)), 1); let mint_ref = &borrow_global<ManagingRefs>(signer::address_of(account)).mint_ref; let fa = fungible_asset::mint(mint_ref, amount); primary_fungible_store::deposit(to, fa); }Likely an incorrect or invalid review comment.
x/move/keeper/bank.go
Outdated
| // if it is not a dispatchable fungible asset, return supply from supply store | ||
| hasDispatchSupplyStore, err := k.HasDispatchSupplyStore(ctx, metadata) | ||
| if err != nil { | ||
| return sdkmath.ZeroInt(), err | ||
| } else if !hasDispatchSupplyStore { | ||
| bz, err := k.GetResourceBytes(ctx, metadata, vmtypes.StructTag{ | ||
| Address: vmtypes.StdAddress, | ||
| Module: types.MoveModuleNameFungibleAsset, | ||
| Name: types.ResourceNameSupply, | ||
| TypeArgs: []vmtypes.TypeTag{}, | ||
| }) | ||
| if err != nil && errors.Is(err, collections.ErrNotFound) { | ||
| return sdkmath.ZeroInt(), nil | ||
| } else if err != nil { | ||
| return sdkmath.ZeroInt(), err | ||
| } | ||
|
|
||
| num, err := types.ReadSupplyFromSupply(bz) | ||
| if err != nil { | ||
| return sdkmath.ZeroInt(), err | ||
| } | ||
|
|
||
| return num, nil | ||
| } | ||
|
|
||
| num, err := types.ReadSupplyFromSupply(bz) | ||
| // use limited gas for dispatchable fungible assets supply query | ||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
| gasMeter := sdkCtx.GasMeter() | ||
|
|
||
| const getSupplyMaxGas = storetypes.Gas(100000) | ||
| sdkCtx = sdkCtx.WithGasMeter(storetypes.NewGasMeter(min(gasMeter.GasRemaining(), getSupplyMaxGas))) | ||
| defer func() { | ||
| usedGas := sdkCtx.GasMeter().GasConsumedToLimit() | ||
| gasMeter.ConsumeGas(usedGas, "GetSupply") | ||
| }() | ||
|
|
||
| // query balance from primary_fungible_store | ||
| output, _, err := k.ExecuteViewFunctionJSON( | ||
| ctx, | ||
| vmtypes.StdAddress, | ||
| types.MoveModuleNameDispatchableFungibleAsset, | ||
| types.FunctionNameDispatchableFungibleAssetDerivedSupply, | ||
| []vmtypes.TypeTag{ | ||
| &vmtypes.TypeTag__Struct{ | ||
| Value: vmtypes.StructTag{ | ||
| Address: vmtypes.StdAddress, | ||
| Module: types.MoveModuleNameFungibleAsset, | ||
| Name: types.ResourceNameMetadata, | ||
| TypeArgs: []vmtypes.TypeTag{}, | ||
| }, | ||
| }}, | ||
| []string{fmt.Sprintf("\"%s\"", metadata)}, | ||
| ) | ||
| if err != nil { | ||
| // ignore fetching error due to dispatchable fungible assets | ||
| return sdkmath.ZeroInt(), nil | ||
| } | ||
|
|
||
| var optionSupplyStr string | ||
| if err := json.Unmarshal([]byte(output.Ret), &optionSupplyStr); err != nil { | ||
| return sdkmath.ZeroInt(), err | ||
| } else if optionSupplyStr == "null" { | ||
| // return is option, so return zero if it is null | ||
| return sdkmath.ZeroInt(), nil | ||
| } | ||
|
|
||
| supply, ok := sdkmath.NewIntFromString(optionSupplyStr) | ||
| if !ok { | ||
| return sdkmath.ZeroInt(), moderrors.Wrapf(types.ErrInvalidResponse, "invalid supply: %s", optionSupplyStr) | ||
| } | ||
|
|
||
| return num, nil | ||
| return supply, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same gas-capping propagation issue in GetSupplyWithMetadata
The method repeats the context-propagation bug highlighted above; supply queries will still consume unlimited gas.
Apply the same fix: wrap the modified sdkCtx back into a context.Context and use that variable for the subsequent ExecuteViewFunctionJSON call.
🤖 Prompt for AI Agents
In x/move/keeper/bank.go around lines 243 to 315, the gas-capping fix is
incomplete because the modified sdkCtx with the limited gas meter is not wrapped
back into a context.Context and passed to ExecuteViewFunctionJSON. To fix this,
wrap the updated sdkCtx into a new context.Context variable and use that wrapped
context in the ExecuteViewFunctionJSON call to ensure gas limits are enforced
during the supply query.
x/move/keeper/bank.go
Outdated
| // use limited gas for dispatchable fungible assets balance query | ||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
| gasMeter := sdkCtx.GasMeter() | ||
|
|
||
| const getBalanceMaxGas = storetypes.Gas(100000) | ||
| sdkCtx = sdkCtx.WithGasMeter(storetypes.NewGasMeter(min(gasMeter.GasRemaining(), getBalanceMaxGas))) | ||
| defer func() { | ||
| usedGas := sdkCtx.GasMeter().GasConsumedToLimit() | ||
| gasMeter.ConsumeGas(usedGas, "GetBalance") | ||
| }() | ||
|
|
||
| // query balance from primary_fungible_store |
There was a problem hiding this comment.
Limited-gas context is never propagated → gas cap is ineffective
WithGasMeter returns a new sdk.Context.
Because the subsequent ExecuteViewFunctionJSON call still uses the original ctx, the view-function runs with un-capped gas, and the defer block will always observe 0 gas consumption. The same mistake exists in GetSupplyWithMetadata.
- sdkCtx = sdkCtx.WithGasMeter(storetypes.NewGasMeter(min(gasMeter.GasRemaining(), getBalanceMaxGas)))
+ sdkCtx = sdkCtx.WithGasMeter(storetypes.NewGasMeter(min(gasMeter.GasRemaining(), getBalanceMaxGas)))
+
+ // propagate capped context downstream
+ ctx = sdk.WrapSDKContext(sdkCtx)Then pass the updated ctx to ExecuteViewFunctionJSON.
Remember to apply the same pattern in GetSupplyWithMetadata.
🤖 Prompt for AI Agents
In x/move/keeper/bank.go around lines 62 to 73, the updated sdk.Context with the
limited gas meter is created but not used in the subsequent call, so the gas cap
is ineffective. Fix this by assigning the new context returned from WithGasMeter
to a variable and passing this updated context to ExecuteViewFunctionJSON
instead of the original ctx. Apply the same fix to the GetSupplyWithMetadata
function to ensure the limited-gas context is properly propagated there as well.
| func Test_DispatchableToken(t *testing.T) { | ||
| ctx, input := createDefaultTestInput(t) | ||
|
|
||
| // TODO - remove this after movevm version update | ||
| //////////////////////////////////////////////////// | ||
| moduleBytes, err := v1_1_1.GetModuleBytes() | ||
| require.NoError(t, err) | ||
|
|
||
| var modules []vmtypes.Module | ||
| for _, module := range moduleBytes { | ||
| modules = append(modules, vmtypes.NewModule(module)) | ||
| } | ||
|
|
||
| err = input.MoveKeeper.PublishModuleBundle(ctx, vmtypes.StdAddress, vmtypes.NewModuleBundle(modules...), types.UpgradePolicy_COMPATIBLE) | ||
| require.NoError(t, err) | ||
| //////////////////////////////////////////////////// | ||
| deployer, err := vmtypes.NewAccountAddress("0xcafe") | ||
| require.NoError(t, err) | ||
|
|
||
| moveBankKeeper := input.MoveKeeper.MoveBankKeeper() | ||
|
|
||
| err = input.MoveKeeper.PublishModuleBundle(ctx, deployer, vmtypes.NewModuleBundle(vmtypes.NewModule(dispatchableTokenModule)), types.UpgradePolicy_COMPATIBLE) | ||
| require.NoError(t, err) | ||
|
|
||
| // execute initialize | ||
| err = input.MoveKeeper.ExecuteEntryFunctionJSON(ctx, deployer, deployer, "test_dispatchable_token", "initialize", []vmtypes.TypeTag{}, []string{}) | ||
| require.NoError(t, err) | ||
|
|
||
| // check supply | ||
| metadata := types.NamedObjectAddress(deployer, "test_token") | ||
| supply, err := moveBankKeeper.GetSupplyWithMetadata(ctx, metadata) | ||
| require.NoError(t, err) | ||
| require.Equal(t, sdkmath.NewInt(0), supply) | ||
|
|
||
| // mint token | ||
| err = input.MoveKeeper.ExecuteEntryFunctionJSON(ctx, deployer, deployer, "test_dispatchable_token", "mint", []vmtypes.TypeTag{}, []string{fmt.Sprintf("\"%s\"", deployer.String()), `"1000000"`}) | ||
| require.NoError(t, err) | ||
|
|
||
| // get supply | ||
| supply, err = moveBankKeeper.GetSupplyWithMetadata(ctx, metadata) | ||
| require.NoError(t, err) | ||
| require.Equal(t, sdkmath.NewInt(1_000_000).MulRaw(10), supply) | ||
|
|
||
| // get balance | ||
| denom, err := types.DenomFromMetadataAddress(ctx, moveBankKeeper, metadata) | ||
| require.NoError(t, err) | ||
| balance, err := moveBankKeeper.GetBalance(ctx, deployer[:], denom) | ||
| require.NoError(t, err) | ||
| require.Equal(t, sdkmath.NewInt(1_000_000).MulRaw(10), balance) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type mismatch when calling GetBalance
deployer[:] is a []byte; GetBalance expects sdk.AccAddress.
-balance, err := moveBankKeeper.GetBalance(ctx, deployer[:], denom)
+balance, err := moveBankKeeper.GetBalance(ctx, sdk.AccAddress(deployer[:]), denom)Without the cast the test will not compile.
📝 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 Test_DispatchableToken(t *testing.T) { | |
| ctx, input := createDefaultTestInput(t) | |
| // TODO - remove this after movevm version update | |
| //////////////////////////////////////////////////// | |
| moduleBytes, err := v1_1_1.GetModuleBytes() | |
| require.NoError(t, err) | |
| var modules []vmtypes.Module | |
| for _, module := range moduleBytes { | |
| modules = append(modules, vmtypes.NewModule(module)) | |
| } | |
| err = input.MoveKeeper.PublishModuleBundle(ctx, vmtypes.StdAddress, vmtypes.NewModuleBundle(modules...), types.UpgradePolicy_COMPATIBLE) | |
| require.NoError(t, err) | |
| //////////////////////////////////////////////////// | |
| deployer, err := vmtypes.NewAccountAddress("0xcafe") | |
| require.NoError(t, err) | |
| moveBankKeeper := input.MoveKeeper.MoveBankKeeper() | |
| err = input.MoveKeeper.PublishModuleBundle(ctx, deployer, vmtypes.NewModuleBundle(vmtypes.NewModule(dispatchableTokenModule)), types.UpgradePolicy_COMPATIBLE) | |
| require.NoError(t, err) | |
| // execute initialize | |
| err = input.MoveKeeper.ExecuteEntryFunctionJSON(ctx, deployer, deployer, "test_dispatchable_token", "initialize", []vmtypes.TypeTag{}, []string{}) | |
| require.NoError(t, err) | |
| // check supply | |
| metadata := types.NamedObjectAddress(deployer, "test_token") | |
| supply, err := moveBankKeeper.GetSupplyWithMetadata(ctx, metadata) | |
| require.NoError(t, err) | |
| require.Equal(t, sdkmath.NewInt(0), supply) | |
| // mint token | |
| err = input.MoveKeeper.ExecuteEntryFunctionJSON(ctx, deployer, deployer, "test_dispatchable_token", "mint", []vmtypes.TypeTag{}, []string{fmt.Sprintf("\"%s\"", deployer.String()), `"1000000"`}) | |
| require.NoError(t, err) | |
| // get supply | |
| supply, err = moveBankKeeper.GetSupplyWithMetadata(ctx, metadata) | |
| require.NoError(t, err) | |
| require.Equal(t, sdkmath.NewInt(1_000_000).MulRaw(10), supply) | |
| // get balance | |
| denom, err := types.DenomFromMetadataAddress(ctx, moveBankKeeper, metadata) | |
| require.NoError(t, err) | |
| balance, err := moveBankKeeper.GetBalance(ctx, deployer[:], denom) | |
| require.NoError(t, err) | |
| require.Equal(t, sdkmath.NewInt(1_000_000).MulRaw(10), balance) | |
| } | |
| func Test_DispatchableToken(t *testing.T) { | |
| ctx, input := createDefaultTestInput(t) | |
| // TODO - remove this after movevm version update | |
| //////////////////////////////////////////////////// | |
| moduleBytes, err := v1_1_1.GetModuleBytes() | |
| require.NoError(t, err) | |
| var modules []vmtypes.Module | |
| for _, module := range moduleBytes { | |
| modules = append(modules, vmtypes.NewModule(module)) | |
| } | |
| err = input.MoveKeeper.PublishModuleBundle(ctx, vmtypes.StdAddress, vmtypes.NewModuleBundle(modules...), types.UpgradePolicy_COMPATIBLE) | |
| require.NoError(t, err) | |
| //////////////////////////////////////////////////// | |
| deployer, err := vmtypes.NewAccountAddress("0xcafe") | |
| require.NoError(t, err) | |
| moveBankKeeper := input.MoveKeeper.MoveBankKeeper() | |
| err = input.MoveKeeper.PublishModuleBundle(ctx, deployer, vmtypes.NewModuleBundle(vmtypes.NewModule(dispatchableTokenModule)), types.UpgradePolicy_COMPATIBLE) | |
| require.NoError(t, err) | |
| // execute initialize | |
| err = input.MoveKeeper.ExecuteEntryFunctionJSON(ctx, deployer, deployer, "test_dispatchable_token", "initialize", []vmtypes.TypeTag{}, []string{}) | |
| require.NoError(t, err) | |
| // check supply | |
| metadata := types.NamedObjectAddress(deployer, "test_token") | |
| supply, err := moveBankKeeper.GetSupplyWithMetadata(ctx, metadata) | |
| require.NoError(t, err) | |
| require.Equal(t, sdkmath.NewInt(0), supply) | |
| // mint token | |
| err = input.MoveKeeper.ExecuteEntryFunctionJSON(ctx, deployer, deployer, "test_dispatchable_token", "mint", []vmtypes.TypeTag{}, []string{fmt.Sprintf("\"%s\"", deployer.String()), `"1000000"`}) | |
| require.NoError(t, err) | |
| // get supply | |
| supply, err = moveBankKeeper.GetSupplyWithMetadata(ctx, metadata) | |
| require.NoError(t, err) | |
| require.Equal(t, sdkmath.NewInt(1_000_000).MulRaw(10), supply) | |
| // get balance | |
| denom, err := types.DenomFromMetadataAddress(ctx, moveBankKeeper, metadata) | |
| require.NoError(t, err) | |
| balance, err := moveBankKeeper.GetBalance(ctx, sdk.AccAddress(deployer[:]), denom) | |
| require.NoError(t, err) | |
| require.Equal(t, sdkmath.NewInt(1_000_000).MulRaw(10), balance) | |
| } |
🤖 Prompt for AI Agents
In x/move/keeper/bank_test.go around lines 329 to 378, the call to GetBalance
uses deployer[:] which is a []byte, but GetBalance expects an sdk.AccAddress
type. To fix this, convert deployer to sdk.AccAddress by casting it
appropriately before passing it to GetBalance, ensuring the test compiles
correctly.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #412 +/- ##
==========================================
+ Coverage 40.33% 40.43% +0.09%
==========================================
Files 296 297 +1
Lines 27879 27912 +33
==========================================
+ Hits 11246 11286 +40
+ Misses 14897 14892 -5
+ Partials 1736 1734 -2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
x/move/keeper/bank_test.go (1)
373-377: Type mismatch when passingdeployer[:]toGetBalance
GetBalanceexpects ansdk.AccAddress, butdeployer[:]is a[]byte.
Cast it explicitly to avoid compilation failure:-balance, err := moveBankKeeper.GetBalance(ctx, deployer[:], denom) +balance, err := moveBankKeeper.GetBalance(ctx, sdk.AccAddress(deployer[:]), denom)Same issue exists in
Test_InvalidDispatchableToken.Also applies to: 424-427
🧹 Nitpick comments (3)
x/move/keeper/bank_test.go (1)
332-344: Refactor repeated “upgrade module bundle” bootstrap into a shared test helperThe 13-line pre-amble that publishes the
v1_1_1upgrade bundle is copy-pasted in bothTest_DispatchableTokenandTest_InvalidDispatchableToken.
Duplicated set-up code makes the tests longer, harder to read, and increases the maintenance burden when the bootstrap logic changes (e.g. once the TODO is removed).Suggestion:
-// TODO - remove this after movevm version update -//////////////////////////////////////////////////// -moduleBytes, err := v1_1_1.GetModuleBytes() -... -require.NoError(t, err) -//////////////////////////////////////////////////// +moduleBundle, err := prepareUpgradeBundle(t) // helper in common_test.go +require.NoError(t, err) + +err = input.MoveKeeper.PublishModuleBundle( + ctx, vmtypes.StdAddress, moduleBundle, types.UpgradePolicy_COMPATIBLE, +) +require.NoError(t, err)Where
prepareUpgradeBundlereturnsvmtypes.ModuleBundle.This keeps each test focused on its unique behaviour.
Also applies to: 383-395
x/move/keeper/contracts/sources/InvalidDispatchableToken.move (2)
55-63: Unusedburn_refandtransfer_ref– consider omitting or exposing use-cases
burn_refandtransfer_refare generated and stored but never read or exposed by this module.
If they are intentionally unused (because this is an “invalid” token), adding a brief comment would prevent future readers from treating this as dead code; otherwise, remove them to save 8 bytes of global storage per deployer.
66-78: Document the intentional infinite loops
derived_balanceandderived_supplycontainloop { … }, which will consume all gas and always abort, relying on the keeper’s timeout logic.
Although this is deliberate for a negative-test module, add a short comment explaining the purpose; this protects future maintainers (and static-analysis tools) from filing bugs unnecessarily.Example:
/// Intentionally non-terminating to simulate a faulty dispatch function. /// The keeper should cap gas and treat this as returning zero. loop { fungible_asset::balance(store); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
x/move/keeper/bank.go(5 hunks)x/move/keeper/bank_test.go(2 hunks)x/move/keeper/common_test.go(2 hunks)x/move/keeper/contracts/sources/InvalidDispatchableToken.move(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/move/keeper/common_test.go
- x/move/keeper/bank.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Analyze (go)
Description
limit gas amount for balance query and supply query
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...