refactor(x/escrow): truncate dec part during payment withdraw #2024
Conversation
WalkthroughRefactors test mock setup with TestSuite.PrepareMocks, updates escrow and market hooks/keeper methods to return errors, implements v1.1.0 upgrade to close overdrawn escrow accounts, adds an App codec field and accessor, and updates tests, configs, and upgrade tooling. Changes
Sequence Diagram(s)sequenceDiagram
actor Test
participant PrepareMocks
participant MockRegistry
participant BankKeeper
participant StateOp as State Operation
Note over Test,StateOp: Old Pattern — Blanket mocks in suite setup
Test->>MockRegistry: Register global BankKeeper mocks
Test->>StateOp: Run test scenario
StateOp->>BankKeeper: Call transfer method
BankKeeper-->>StateOp: Mocked response
Note over Test,StateOp: New Pattern — Deferred per-test mocks
Test->>PrepareMocks: Call PrepareMocks(fn)
PrepareMocks->>MockRegistry: Register test-specific BankKeeper mocks
Test->>StateOp: Run test scenario
StateOp->>BankKeeper: Call transfer method
BankKeeper-->>StateOp: Mocked response
sequenceDiagram
participant Upgrade as v1.1.0 Upgrade
participant EscrowKeeper
participant Account as Escrow Account
participant Payment as Escrow Payment
participant Events
Upgrade->>EscrowKeeper: closeOverdrawnEscrowAccounts()
loop each overdrawn account
EscrowKeeper->>Account: Load account
EscrowKeeper->>Payment: Gather payments
EscrowKeeper->>EscrowKeeper: Compute owed amounts
EscrowKeeper->>Account: Update and persist state
EscrowKeeper->>Payment: Update/persist payments
EscrowKeeper->>Events: Trigger OnEscrowAccountClosed
Events-->>Upgrade: propagate any error
opt success
Events->>Upgrade: Trigger OnGroupClosed / OnEscrowPaymentClosed flows
end
end
Upgrade-->>EscrowKeeper: Completed cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
testutil/state/suite.go(2 hunks)x/deployment/keeper/grpc_query_test.go(8 hunks)x/escrow/keeper/grpc_query_test.go(4 hunks)x/escrow/keeper/keeper.go(4 hunks)x/escrow/keeper/keeper_settle_test.go(2 hunks)x/escrow/keeper/keeper_test.go(9 hunks)x/market/handler/handler_test.go(12 hunks)x/market/keeper/grpc_query_test.go(7 hunks)x/market/keeper/keeper_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
x/market/keeper/grpc_query_test.go (1)
testutil/state/suite.go (1)
TestSuite(42-48)
x/market/keeper/keeper_test.go (1)
x/escrow/keeper/external.go (1)
BankKeeper(12-18)
x/deployment/keeper/grpc_query_test.go (2)
testutil/state/suite.go (1)
TestSuite(42-48)x/escrow/keeper/external.go (1)
BankKeeper(12-18)
x/market/handler/handler_test.go (2)
testutil/state/suite.go (1)
TestSuite(42-48)x/market/handler/keepers.go (1)
BankKeeper(56-59)
x/escrow/keeper/grpc_query_test.go (1)
testutil/state/suite.go (1)
TestSuite(42-48)
🪛 golangci-lint (2.5.0)
x/escrow/keeper/keeper_settle_test.go
[error] 104-104: commentFormatting: put a space between // and comment text
(gocritic)
⏰ 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). (8)
- GitHub Check: network-upgrade
- GitHub Check: sims
- GitHub Check: release-dry-run
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: build-bins
- GitHub Check: build-macos
🔇 Additional comments (17)
x/escrow/keeper/keeper.go (3)
987-987: Verify TruncateDec() prevents fractional accumulation.The addition of
TruncateDec()truncates the decimal portion of the payment calculation to address fractional uakt remaining after account closure. This correctly prevents fractional amounts from accumulating in balances.
995-998: Critical fix: Correct unsettled amount calculation.Line 998 now correctly adjusts
unsettledAmountby subtractingsettledAmount.Amountinstead of subtracting its own value (which would incorrectly zero it out). This ensures that when partial settlement occurs (e.g., due to insufficient funds), the unsettled portion accurately reflects the difference between requested and actual settled amounts.
957-962: LGTM: Improved deductFromBalance logic.The refactoring correctly:
- Cleans up empty deposits (idx > 0) after processing withdrawals
- Subtracts the actual withdrawn amount (
withdrew) from funds instead of the requested amount (toWithdraw)This ensures funds.Amount accurately reflects the actual deduction rather than the requested amount, which is especially important in overdraw scenarios.
testutil/state/suite.go (2)
79-80: LGTM: Deferred mock configuration improves test control.Removing pre-configured bank mock expectations from suite setup and adding the comment correctly explains the rationale. This allows individual tests to configure mocks based on their specific scenarios, ensuring escrow balance values are tracked accurately.
165-167: LGTM: Simple and effective helper method.The
PrepareMockshelper provides a clean pattern for tests to configure mocks after suite initialization. The implementation is straightforward and consistent with the broader test refactoring.x/escrow/keeper/keeper_test.go (2)
20-75: LGTM: Comprehensive settlement test validates the fix.The new
Test_AccountSettlementtest validates the complete settlement flow:
- Creates escrow account with 1000 uakt deposit
- Creates payment with 10 uakt/block rate
- Advances 10 blocks (100 uakt total payment)
- Verifies correct distribution: 2 uakt fee (2%), 98 uakt to provider, 900 uakt refund
- Confirms settled amount is truncated to prevent fractional balances
The test expectations correctly validate the TruncateDec() fix and proper fund distribution.
108-114: Note: Account closure now processes deposits individually.The comment "each deposit is it's own send" and the updated mock expectations (separate
SendCoinsFromModuleToAccountcalls foramtandamt2) reflect that account closure now processes and returns each deposit separately rather than as a combined amount. This aligns with the keeper logic changes.x/market/keeper/keeper_test.go (1)
371-383: LGTM: Consistent mock setup pattern.The
PrepareMocksusage correctly configures bank keeper mocks to return nil for coin transfer operations. This pattern is consistently applied across test files to enable deterministic testing without actual coin transfers.x/market/handler/handler_test.go (1)
83-95: LGTM: Mock setup enables isolated bid creation testing.The PrepareMocks configuration properly stubs all bank keeper methods used during bid creation flows, ensuring tests can validate business logic without side effects from actual coin transfers.
x/escrow/keeper/grpc_query_test.go (1)
270-282: LGTM: Escrow account creation tests properly mocked.The mock setup in
createEscrowAccountcorrectly stubs bank transfer methods, aligning with the pattern used throughout the test suite refactoring.x/market/keeper/grpc_query_test.go (1)
420-432: LGTM: Query tests properly isolated with mocks.The PrepareMocks setup before bid/lease filtering tests ensures queries execute without triggering actual bank operations, maintaining test isolation.
x/deployment/keeper/grpc_query_test.go (1)
66-78: LGTM: Deployment test mocks consistent with suite pattern.Both the test setup and
createDeploymenthelper properly configure bank keeper mocks, ensuring consistent mock behavior across deployment-related test scenarios.Also applies to: 583-595
x/escrow/keeper/keeper_settle_test.go (5)
4-4: LGTM: Import is necessary for enhanced assertion messages.The
fmtimport is used correctly in the new assertion messages at lines 116 and 131.
111-116: LGTM: Transferred amounts are correctly validated.The logic correctly accumulates all transferred amounts and verifies they match the account's transferred state. The assertion message clearly describes the validation.
118-126: LGTM: Payment tracking correctly accounts for settled and unsettled amounts.The accumulation logic properly tracks both settled (Balance) and unsettled amounts. The
totalPaymentsvariable captures the complete payment obligation, which is essential for accurate balance verification in both normal and overdrawn scenarios.
128-132: LGTM: Balance verification ensures correct fund deduction.The calculation correctly verifies that the account balance was decremented by the total payment obligations (including unsettled amounts). This is a valuable assertion for detecting fund tracking bugs.
133-140: LGTM: Overdrawn reconciliation correctly validates unsettled tracking.The overdrawn validation ensures that negative balances are properly reconciled with unsettled amounts. This is crucial for preventing balance discrepancies when accounts are closed, directly addressing the PR's stated objective.
| }, | ||
| } { | ||
| account, payments, blocks := setupDistTest(t, tt.cfg) | ||
| //initialBalance := sdkmath.LegacyMustNewDecFromStr(account.State.Deposits[0].Balance.Amount.String()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out dead code.
This commented-out line is unused and should be removed to keep the codebase clean. If there's a reason to preserve it for reference, it should be properly formatted with a space after //.
As per coding guidelines.
Apply this diff:
- //initialBalance := sdkmath.LegacyMustNewDecFromStr(account.State.Deposits[0].Balance.Amount.String())📝 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.
| //initialBalance := sdkmath.LegacyMustNewDecFromStr(account.State.Deposits[0].Balance.Amount.String()) |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 104-104: commentFormatting: put a space between // and comment text
(gocritic)
🤖 Prompt for AI Agents
In x/escrow/keeper/keeper_settle_test.go around line 104, there is a leftover
commented-out line ("//initialBalance :=
sdkmath.LegacyMustNewDecFromStr(account.State.Deposits[0].Balance.Amount.String())")
that should be removed to eliminate dead code; delete the entire commented line
(or if it must be kept for an explicit reason, reformat it with a space after
"//" and add a comment explaining why it remains) and run go fmt/tests to ensure
no formatting issues.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
x/escrow/keeper/keeper_settle_test.go (1)
104-104: Remove commented‑out dead code to satisfy linterThe commented‑out
initialBalanceline is unused and is still trippinggolangci-lint(commentFormatting: put a space between // and comment text). Since it’s not referenced anymore, it should be removed rather than kept as a comment.You can simply delete the line:
- //initialBalance := sdkmath.LegacyMustNewDecFromStr(account.State.Deposits[0].Balance.Amount.String())This also addresses the prior review comment on the same line.
🧹 Nitpick comments (8)
x/market/handler/handler_test.go (2)
1429-1450: Remove or utilize the unused helper function.Static analysis correctly identifies that
createDeploymentis defined but never called. Either remove this helper if it's not needed, or use it in the tests where appropriate to reduce code duplication (e.g., in the three full-flow tests that currently inline deployment creation logic).
82-104: Consider utilizing the helper to reduce duplication.The three full-flow tests contain similar deployment setup code. The
createDeploymenthelper at line 1429 could be used to reduce this duplication, though the current inline approach may be preferred for test readability. If you choose to keep the inline approach, consider removing the unused helper function.Also applies to: 388-410, 626-650
x/deployment/handler/handler_test.go (1)
443-443: Fix comment formatting or remove commented code.The commented-out code lacks a space after
//. Either add the space (// owner := ...) or remove the commented code if it's no longer needed.x/escrow/genesis.go (1)
75-80: Good:SavePaymenterrors no longer ignored; consider enriching panic contextPropagating failures from
keeper.SavePaymentand aborting genesis on error is the right move for data integrity. The only small improvement would be to include some payment context (e.g. index or ID) and avoid callingerr.Error()explicitly:-for idx := range data.Payments { - err := keeper.SavePayment(ctx, data.Payments[idx]) - if err != nil { - panic(fmt.Sprintf("error saving payment: %s", err.Error())) - } -} +for idx := range data.Payments { + if err := keeper.SavePayment(ctx, data.Payments[idx]); err != nil { + panic(fmt.Sprintf("error saving payment %d (%s): %v", idx, data.Payments[idx].ID, err)) + } +}This keeps the behavior but makes debugging a bad genesis much easier.
x/market/keeper/keeper.go (1)
351-369: Clean up deaderrcheck and clarifyPaymentCloseerror handling inOnGroupClosedWithin
processClose:err := k.OnBidClosed(ctx, bid) if err != nil { return err } if lease, ok := k.GetLease(ctx, bid.ID.LeaseID()); ok { err = k.OnLeaseClosed(ctx, lease, mv1.LeaseClosed, mv1.LeaseClosedReasonOwner) if err != nil { return err } if err := k.ekeeper.PaymentClose(ctx, lease.ID.ToEscrowPaymentID()); err != nil { ctx.Logger().With("err", err).Info("error closing payment") } if err != nil { return err } }
- The final
if err != nil { return err }afterPaymentCloseis effectively dead:errthere is the same local variable used forOnBidClosed/OnLeaseClosed, and both earlier checks already return on non-nil;PaymentCloseuses its own short-livederrin theifinitializer, so this trailing check never sees a non-nil error. It’s misleading and should be removed.- Currently,
PaymentClosefailures are logged but do not causeOnGroupClosedto fail. That may be intentional (best-effort cleanup) but is worth confirming, since it affects how callers reason about closure vs. residual escrow state.A clearer version would be:
- err = k.OnLeaseClosed(ctx, lease, mv1.LeaseClosed, mv1.LeaseClosedReasonOwner) - if err != nil { - return err - } - if err := k.ekeeper.PaymentClose(ctx, lease.ID.ToEscrowPaymentID()); err != nil { - ctx.Logger().With("err", err).Info("error closing payment") - } - if err != nil { - return err - } + if err := k.OnLeaseClosed(ctx, lease, mv1.LeaseClosed, mv1.LeaseClosedReasonOwner); err != nil { + return err + } + if err := k.ekeeper.PaymentClose(ctx, lease.ID.ToEscrowPaymentID()); err != nil { + // Either keep this as best-effort logging... + ctx.Logger().With("err", err).Info("error closing payment") + // ...or, if desired, return err here to surface payment-close failures. + }This removes the dead check and makes the intended semantics around
PaymentCloseexplicit.Please confirm whether
OnGroupClosedshould fail hard onPaymentCloseerrors or treat them as best-effort cleanup; the current code path logs but does not propagate such errors.x/escrow/keeper/grpc_query_test.go (1)
269-282: Centralize BankKeeper SendCoins mocks to reduce duplication and clarify lifecycleThe
PrepareMocksblock stubbing all threeSendCoins*methods tonilis appropriate for isolating escrow-account creation from real bank transfers. However, the same triple of stubs appears in other test files in this PR.Consider extracting a small helper (in
state.TestSuiteor a shared test helper) that configures these BankKeeper mocks once, and then calling it from here and other tests. That would:
- Avoid copy-paste when these signatures change.
- Make it clearer when/where BankKeeper behavior is altered for a test.
If
PrepareMocksaccumulates callbacks rather than replacing them, also confirm that repeated calls don’t unintentionally stack expectations across tests.x/deployment/keeper/grpc_query_test.go (1)
63-79: Deduplicate BankKeeper SendCoins mocking across tests and helpersEach of these blocks configures the same three
SendCoins*methods on the BankKeeper mock to returnnil, which is exactly what you want for isolation from bank side effects. The downside is that the same stub logic is repeated in multiple tests pluscreateDeployment, andTestGRPCQueryDeploymentmay effectively apply it twice (in the test and again insidecreateDeployment).Consider:
- Extracting a small helper (e.g.
prepareBankSendMocks(ts *state.TestSuite)) and calling it from each test/helper.- Or, if
PrepareMocksis designed to be cumulative, centralizing this configuration in one place per suite.This would reduce duplication and make future changes to BankKeeper behavior easier to propagate without missing a call site.
Also applies to: 154-168, 287-301, 493-508, 580-595
tests/upgrade/sdktypes.go (1)
13-40: Unused helper types may trip golangci‑lint’sunusedcheck
nodeStatus,votingParams,depositParams,govParams,proposalResp, andproposalsRespare all unexported and currently unused within theupgradepackage, which matches the static‑analysis warnings.If these are only placeholders for future work, consider one of:
- Wiring them into the actual upgrade tests that decode these JSON payloads.
- Removing them until they’re needed.
- Or, if they’re intentionally kept for clarity, annotating them with
//nolint:unused(and possibly a short comment) sogolangci-lintdoesn’t fail.The exported
SoftwareUpgradeProposalandProposalMsgtypes are fine as-is.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
app/app.go(1 hunks)app/types/app.go(2 hunks)go.mod(1 hunks)make/test-upgrade.mk(3 hunks)meta.json(1 hunks)tests/upgrade/sdktypes.go(1 hunks)tests/upgrade/test-cases.json(3 hunks)tests/upgrade/test-config.json(1 hunks)tests/upgrade/types/types.go(1 hunks)tests/upgrade/upgrade_test.go(14 hunks)tests/upgrade/workers_test.go(1 hunks)testutil/state/suite.go(2 hunks)upgrades/software/v1.1.0/init.go(1 hunks)upgrades/software/v1.1.0/upgrade.go(1 hunks)upgrades/upgrades.go(1 hunks)x/deployment/handler/handler_test.go(15 hunks)x/deployment/keeper/grpc_query_test.go(8 hunks)x/escrow/genesis.go(1 hunks)x/escrow/keeper/grpc_query.go(3 hunks)x/escrow/keeper/grpc_query_test.go(4 hunks)x/escrow/keeper/keeper.go(23 hunks)x/escrow/keeper/keeper_settle_test.go(2 hunks)x/escrow/keeper/keeper_test.go(10 hunks)x/market/handler/handler_test.go(16 hunks)x/market/hooks/hooks.go(3 hunks)x/market/keeper/grpc_query_test.go(7 hunks)x/market/keeper/keeper.go(1 hunks)x/market/keeper/keeper_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/market/keeper/keeper_test.go
- x/market/keeper/grpc_query_test.go
🧰 Additional context used
🧬 Code graph analysis (9)
x/escrow/keeper/grpc_query.go (1)
x/escrow/keeper/key.go (2)
AccountPrefix(20-20)PaymentPrefix(21-21)
tests/upgrade/workers_test.go (1)
tests/upgrade/types/types.go (1)
RegisterPostUpgradeWorker(38-44)
x/deployment/keeper/grpc_query_test.go (4)
testutil/state/suite.go (1)
TestSuite(42-48)x/escrow/keeper/external.go (1)
BankKeeper(12-18)x/deployment/handler/keepers.go (1)
BankKeeper(38-40)x/market/handler/keepers.go (1)
BankKeeper(56-59)
x/escrow/keeper/keeper.go (1)
x/escrow/keeper/key.go (2)
BuildPaymentsKey(50-56)ParsePaymentKey(122-160)
x/deployment/handler/handler_test.go (3)
testutil/cosmos/keepers.go (1)
BankKeeper(12-18)x/escrow/keeper/external.go (1)
BankKeeper(12-18)x/deployment/handler/keepers.go (1)
BankKeeper(38-40)
x/escrow/keeper/keeper_test.go (2)
testutil/cosmos/keepers.go (1)
BankKeeper(12-18)x/market/keeper/external.go (1)
EscrowKeeper(9-14)
x/market/handler/handler_test.go (5)
testutil/state/suite.go (1)
TestSuite(42-48)x/deployment/handler/handler.go (1)
NewHandler(14-41)x/market/handler/handler.go (1)
NewHandler(12-36)x/market/handler/keepers.go (3)
DeploymentKeeper(43-47)EscrowKeeper(22-30)BankKeeper(56-59)x/deployment/handler/keepers.go (3)
MarketKeeper(19-22)EscrowKeeper(24-29)BankKeeper(38-40)
upgrades/software/v1.1.0/init.go (2)
upgrades/types/types.go (1)
RegisterUpgrade(79-85)upgrades/software/v1.1.0/upgrade.go (1)
UpgradeName(36-36)
x/escrow/keeper/grpc_query_test.go (1)
testutil/state/suite.go (1)
TestSuite(42-48)
🪛 golangci-lint (2.5.0)
x/escrow/keeper/keeper_settle_test.go
[error] 104-104: commentFormatting: put a space between // and comment text
(gocritic)
tests/upgrade/sdktypes.go
[error] 13-13: type nodeStatus is unused
(unused)
[error] 20-20: type votingParams is unused
(unused)
[error] 24-24: type depositParams is unused
(unused)
[error] 28-28: type govParams is unused
(unused)
[error] 33-33: type proposalResp is unused
(unused)
[error] 38-38: type proposalsResp is unused
(unused)
x/deployment/handler/handler_test.go
[error] 443-443: commentFormatting: put a space between // and comment text
(gocritic)
x/escrow/keeper/keeper_test.go
[error] 20-20: type kTestSuite is unused
(unused)
x/market/handler/handler_test.go
[error] 1429-1429: func (*testSuite).createDeployment is unused
(unused)
⏰ 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). (7)
- GitHub Check: release-dry-run
- GitHub Check: sims
- GitHub Check: lint-go
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: build-bins
- GitHub Check: build-macos
🔇 Additional comments (33)
make/test-upgrade.mk (3)
79-79: The bins invocation in test-reset is intentional and necessary.The
test-resettarget is a composite operation that sequentially prepares the test environment: clean → bins → keys → prepare-state. Thebinscommand at line 79 is an explicit, unconditional step that ensures genesis binaries are available before subsequent operations (keys and prepare-state).The conditional
SKIPlogic in the standalonebinstarget (lines 89-92) applies only when bins is called independently. The direct invocation intest-resetis intentional design—test-reset is a full reset workflow that explicitly requires all steps to execute. This does not conflict with the standalone target's conditional behavior.
67-74: All new test argument dependencies are properly satisfied.
- test-cases.json: Verified to exist at
tests/upgrade/test-cases.json✓- AKASH_ROOT: Defined as an environment variable with a guard in
make/init.mk(lines 17-21) that ensures it is set before any targets execute. If not set, the build fails with a clear error message directing users to rundirenv allow✓- Flag usage: Both
-sourcesdirand-test-casesflags are properly defined and validated intests/upgrade/upgrade_test.go(lines 234, 239, 271, 276) with runtime checks ensuring they are non-empty before test execution ✓The implementation is correct and handles all dependencies safely.
24-24: Clarify the intent behind changing the default snapshot source from sandbox1 to mainnet and ensure CI pipelines are configured accordingly.The default
SNAPSHOT_SOURCEwas changed fromsandbox1tomainnet(line 24). While all three snapshot sources remain supported via conditionals, this change impacts CI/CD pipelines that relied on the lighter sandbox1 default, as mainnet snapshots are significantly larger and slower to download. No explicitSNAPSHOT_SOURCEoverrides were found in CI configurations, meaning they will now use mainnet by default.Verify that:
- This default change aligns with upgrade testing goals (as documented in ADR 001)
- CI pipelines that need faster test execution explicitly set
SNAPSHOT_SOURCE=sandbox1orSNAPSHOT_SOURCE=sandboxif needed- Developers are aware that local test runs will now download mainnet snapshots instead
go.mod (1)
48-48: LGTM: Dependency version update.The bump from v0.1.5 to v0.1.6 is a minor version update that likely includes the changes referenced in this PR.
app/types/app.go (2)
124-124: Public API addition: Codec field.Adding a public
Cdcfield to theAppstruct expands the public API surface. Ensure that this field is properly initialized in all App construction paths and that documentation reflects its intended usage.
196-198: LGTM: Codec accessor method.The
GetCodec()method provides clean, encapsulated access to the codec. This is consistent with the existing pattern of getter methods in the App struct.app/app.go (1)
136-136: LGTM: Codec initialization.The
Cdcfield is properly initialized withappCodec, ensuring the codec is available throughout the App's lifecycle. This aligns with the new field added inapp/types/app.go.testutil/state/suite.go (2)
79-80: LGTM: Updated test setup guidance.The comment clarifies that bank mocks should be configured manually in each test rather than during suite setup. This improves test isolation and makes mock expectations more explicit, aligning with the new
PrepareMockspattern.
165-167: LGTM: Test mock preparation helper.The
PrepareMockshelper provides a clean pattern for configuring mocks on a per-test basis. This improves test clarity by making mock expectations explicit at the point of use rather than hidden in suite setup.x/market/handler/handler_test.go (8)
36-42: LGTM: Enhanced test suite structure.The expanded
testSuitestruct now includes deployment and escrow handlers, enabling comprehensive lifecycle testing across multiple modules. This aligns with the full-flow test scenarios added in this file.
74-378: LGTM: Comprehensive deployment lifecycle test.This test validates the complete deployment lifecycle including creation, bidding, leasing, overdraft handling, and settlement. The extensive mock setup and balance tracking ensure that escrow accounting is correct throughout the flow. The test appropriately covers the PR's objective of handling fractional balances during payment withdrawal.
93-94: Excellent balance tracking pattern.The test maintains in-memory balance tracking through mock callbacks, ensuring that all transfers are accounted for and final balances match expected values. This is crucial for validating the PR's fix for fractional balance handling.
Also applies to: 110-154, 368-377
380-616: LGTM: Lease closure lifecycle test.This test complements the deployment closure test by validating the lease closure path. It correctly tests overdraft scenarios and subsequent deposit-based recovery, ensuring the system handles partial payments correctly.
618-854: LGTM: Bid closure lifecycle test.This test completes the coverage of closure scenarios by testing the bid closure path. The consistent structure across all three full-flow tests (deployment, lease, bid) makes the test suite maintainable and comprehensive.
155-167: Good use of PrepareMocks pattern.The tests consistently use
PrepareMocksto configure bank keeper expectations before each operation. This explicit, localized mock setup improves test clarity and makes it easier to understand what each test step requires.Also applies to: 169-173
874-886: LGTM: Consistent mock pattern in existing tests.The existing tests have been updated to use the
PrepareMockspattern, maintaining consistency across the entire test suite. This refactoring improves test maintainability without changing test logic.
285-291: Well-structured assertions for state validation.The tests include comprehensive assertions that verify both state transitions and exact balance values. This level of precision is appropriate for testing escrow accounting logic where correctness is critical.
Also applies to: 368-377
x/escrow/keeper/grpc_query.go (1)
84-84: LGTM: Function export for external use.Exporting
BuildSearchPrefixenables external components to construct search prefixes consistently. The function signature and implementation remain unchanged; only visibility has been updated. All call sites within this file have been updated appropriately.Also applies to: 196-196, 250-250
x/market/hooks/hooks.go (2)
30-68: LGTM: Proper error propagation in account closure.The
OnEscrowAccountClosedimplementation now correctly propagates errors from deployment closure, group closure, and market operations. The early return pattern ensures that errors are not silently ignored, improving system reliability.
70-117: LGTM: Comprehensive error handling in payment closure.The
OnEscrowPaymentClosedimplementation includes thorough error handling for all closure scenarios, including both normal closure and overdraft conditions. The error propagation from order, bid, and lease operations ensures that failures are properly reported up the call stack.x/deployment/handler/handler_test.go (4)
143-143: LGTM: Improved test function name.The rename from
TestProviderBadMessageTypetoTestHandlerBadMessageTypemore accurately reflects that this is testing the deployment handler's behavior with invalid message types.
157-157: LGTM: Enhanced deployment lifecycle test.The test now validates both deployment creation and closure, with proper mock setup for escrow transfers. The addition of owner extraction and mock preparation aligns with the PR's test pattern improvements.
Also applies to: 172-178, 210-224
283-289: Consistent mock preparation pattern.The tests consistently use
PrepareMocksto configure bank keeper expectations before operations. This aligns with the broader test refactoring across the PR and improves test clarity.Also applies to: 368-374
444-640: Comprehensive funded deployment test.This test thoroughly validates the funded deployment lifecycle, including deposits from multiple sources (owner, grantee, random depositor), payment withdrawals, and multi-recipient refunds on closure. The extensive mock setup is appropriate for this complex scenario.
meta.json (1)
43-52:v1.1.0upgrade metadata matches existing patternThe new
"v1.1.0"block mirrors the existing upgrade entries and lines up with the new v1.1.0 upgrade wiring elsewhere (init.go, upgrades.go). No structural issues spotted here.upgrades/upgrades.go (1)
3-6: Upgrade wiring correctly points tov1.1.0Switching the blank import to
software/v1.1.0is consistent with the new upgrade implementation and init-based registration.tests/upgrade/test-cases.json (1)
2-187: Upgrade test-case entries forv1.1.0and extended v1.0.0 migrations look structurally sound
- Adding the
"v1.1.0"block with emptymodules/migrationsis consistent with the file’s structure and gives you a hook for future per-module migration specs.- The additional
authstep (4 → 5) andtransferstep (3 → 4) extend the migration chains cleanly and appear to fill gaps between existing versions.As long as these step numbers match the actual module version migrations defined in code, this JSON should work as intended with the upgrade test harness.
It’s worth double-checking that the new
from/tovalues line up with the module version constants used in your upgrade handlers.upgrades/software/v1.1.0/init.go (1)
1-11: v1.1.0 upgrade registration matches existing patternThe
init-timeRegisterUpgrade(UpgradeName, initUpgrade)inv1_1_0aligns with the upgrade registry API and the blank import pattern inupgrades/upgrades.go. This is a clean, minimal hook for wiring in the new upgrade.tests/upgrade/test-config.json (1)
3-5: Validator configuration change verified as safeThe
validatorslist now contains only.akash0, which matcheswork.home. Verification confirms no stale references to the removed.akash1validator exist in the tests/upgrade directory.tests/upgrade/types/types.go (1)
11-19:SourceDirfield addition is properly integrated across all call sitesVerification confirms that
SourceDiris explicitly populated at line 398 inupgrade_test.gowithin the initialization loop, alongside all otherTestParamsfields (Home,ChainID,Node,KeyringBackend,From,FromAddress). The struct is correctly initialized and passed through to the test workers. No issues found.x/escrow/keeper/grpc_query_test.go (1)
8-8: Suite embedding and TestSuite wiring look consistentEmbedding
*state.TestSuiteintogrpcTestSuiteand wiring it insetupTestcleanly exposes shared helpers (likePrepareMocks) alongside the existing cached fields. No functional issues stand out; this aligns with the broader test-suite refactor.Also applies to: 28-30, 41-51
x/deployment/keeper/grpc_query_test.go (1)
13-15: Suite embedding and query client setup are coherentAdding the
mockimport, embedding*state.TestSuiteintogrpcTestSuite, and wiring it insetupTestkeeps the deployment tests consistent with other modules’ suites and cleanly exposes the shared mocking utilities while preserving the existing cached fields and query client setup. No functional problems here.Also applies to: 28-39, 41-52
x/escrow/keeper/keeper_settle_test.go (1)
111-140: Stronger settlement accounting invariants look correctThe new aggregates and assertions (
totalTransferred,totalPayments,totalUnsettled,expectedRemainingBalance, and the overdrawn/unsettled checks) are internally consistent with the test configs:
totalTransferredmatches the configured per‑paymenttransferredslice and is asserted againstaccount.State.Transferred[0].Amount.expectedRemainingBalance = balanceStart - totalPayments(wheretotalPaymentsincludes both settled and unsettled amounts) is asserted to equalaccount.State.Funds[0].Amount, tying funds to the sum of payment flows.- In the overdrawn case, verifying that funds are negative and
funds + totalUnsettled == 0enforces a clear invariant between negative balance and accumulated unsettled amounts.This materially tightens the test’s coverage of settlement math without introducing obvious edge‑case issues for the provided scenarios.
| uttypes.RegisterPostUpgradeWorker("v1.1.0", &postUpgrade{}) | ||
| } | ||
|
|
||
| type postUpgrade struct { | ||
| cl v1beta3.Client | ||
| } | ||
| type postUpgrade struct{} |
There was a problem hiding this comment.
🧩 Analysis chain
v1.1.0 post‑upgrade worker is currently a no‑op
Registering "v1.1.0" and keeping postUpgrade as an empty struct satisfies the TestWorker interface but means the v1.1.0 post‑upgrade path performs no checks. If the previous v1.0.0 worker enforced invariants, consider porting or re‑adding those assertions here, or at least adding a TODO so this doesn’t silently stay empty.
Also verify that "v1.1.0" exactly matches the on‑chain upgrade name you expect to exercise.
🏁 Script executed:
#!/bin/bash
# View the workers_test.go file with context around lines 13-16
echo "=== Full workers_test.go file ==="
cat tests/upgrade/workers_test.go
echo -e "\n=== Search for RegisterPostUpgradeWorker calls ==="
rg -n "RegisterPostUpgradeWorker" tests/upgrade/
echo -e "\n=== Search for v1.1.0 references in codebase ==="
rg -n "v1.1.0" --type go
echo -e "\n=== Search for upgrade worker implementations ==="
rg -n "type.*struct.*\{\}" tests/upgrade/ -A 5Length of output: 1956
Add TODO comment to empty v1.1.0 post-upgrade worker
The v1.1.0 postUpgrade worker is confirmed to be a no-op with an empty Run method. The version string "v1.1.0" correctly matches the actual upgrade name. However, since there is no prior v1.0.0 worker to port assertions from, add a TODO comment to the empty Run method explaining why it's currently empty or what checks should be implemented.
func (pu *postUpgrade) Run(ctx context.Context, t *testing.T, params uttypes.TestParams) {
// TODO: add v1.1.0 upgrade invariant checks
}🤖 Prompt for AI Agents
In tests/upgrade/workers_test.go around lines 13 to 16, the registered v1.1.0
postUpgrade worker has an empty Run method; add a TODO comment inside that Run
method stating why it is currently a no-op or what invariant checks should be
implemented (e.g., "// TODO: add v1.1.0 upgrade invariant checks" or similar) so
future reviewers know this is intentional and what to add when relevant.
| heightDelta := ctx.BlockHeight() + val.State.SettledAt | ||
|
|
||
| totalAvailableDeposits := sdkmath.LegacyZeroDec() | ||
|
|
||
| for _, deposit := range val.State.Deposits { | ||
| totalAvailableDeposits.AddMut(deposit.Balance.Amount) | ||
| } | ||
|
|
||
| payments := up.accountPayments(cdc, store, id, []etypes.State{etypes.StateOpen, etypes.StateOverdrawn}) | ||
|
|
||
| totalBlockRate := sdkmath.LegacyZeroDec() | ||
|
|
||
| for _, pmnt := range payments { | ||
| totalBlockRate.AddMut(pmnt.State.Rate.Amount) | ||
|
|
||
| if pmnt.State.State == etypes.StateOverdrawn { | ||
| val.State.State = etypes.StateOverdrawn | ||
| } | ||
| } | ||
|
|
||
| owed := sdkmath.LegacyZeroDec() | ||
| owed.AddMut(totalBlockRate) | ||
| owed.MulInt64Mut(heightDelta) | ||
|
|
||
| overdraft := totalAvailableDeposits.LTE(owed) || val.State.State == etypes.StateOverdrawn | ||
|
|
There was a problem hiding this comment.
Fix the height delta before computing owed balances.
Line 110 currently adds val.State.SettledAt to the current block height. This double-counts the elapsed blocks, so every account looks massively overdue and is forced into the overdrawn path during the upgrade. Switch to a difference (and clamp negatives) so we only charge for the blocks that actually elapsed.
- heightDelta := ctx.BlockHeight() + val.State.SettledAt
+ heightDelta := ctx.BlockHeight() - val.State.SettledAt
+ if heightDelta < 0 {
+ heightDelta = 0
+ }📝 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.
| heightDelta := ctx.BlockHeight() + val.State.SettledAt | |
| totalAvailableDeposits := sdkmath.LegacyZeroDec() | |
| for _, deposit := range val.State.Deposits { | |
| totalAvailableDeposits.AddMut(deposit.Balance.Amount) | |
| } | |
| payments := up.accountPayments(cdc, store, id, []etypes.State{etypes.StateOpen, etypes.StateOverdrawn}) | |
| totalBlockRate := sdkmath.LegacyZeroDec() | |
| for _, pmnt := range payments { | |
| totalBlockRate.AddMut(pmnt.State.Rate.Amount) | |
| if pmnt.State.State == etypes.StateOverdrawn { | |
| val.State.State = etypes.StateOverdrawn | |
| } | |
| } | |
| owed := sdkmath.LegacyZeroDec() | |
| owed.AddMut(totalBlockRate) | |
| owed.MulInt64Mut(heightDelta) | |
| overdraft := totalAvailableDeposits.LTE(owed) || val.State.State == etypes.StateOverdrawn | |
| heightDelta := ctx.BlockHeight() - val.State.SettledAt | |
| if heightDelta < 0 { | |
| heightDelta = 0 | |
| } | |
| totalAvailableDeposits := sdkmath.LegacyZeroDec() | |
| for _, deposit := range val.State.Deposits { | |
| totalAvailableDeposits.AddMut(deposit.Balance.Amount) | |
| } | |
| payments := up.accountPayments(cdc, store, id, []etypes.State{etypes.StateOpen, etypes.StateOverdrawn}) | |
| totalBlockRate := sdkmath.LegacyZeroDec() | |
| for _, pmnt := range payments { | |
| totalBlockRate.AddMut(pmnt.State.Rate.Amount) | |
| if pmnt.State.State == etypes.StateOverdrawn { | |
| val.State.State = etypes.StateOverdrawn | |
| } | |
| } | |
| owed := sdkmath.LegacyZeroDec() | |
| owed.AddMut(totalBlockRate) | |
| owed.MulInt64Mut(heightDelta) | |
| overdraft := totalAvailableDeposits.LTE(owed) || val.State.State == etypes.StateOverdrawn |
🤖 Prompt for AI Agents
In upgrades/software/v1.1.0/upgrade.go around lines 108 to 133, the code
currently computes heightDelta by adding val.State.SettledAt to
ctx.BlockHeight(), which double-counts elapsed blocks; change this to compute
heightDelta = ctx.BlockHeight() - val.State.SettledAt and clamp to zero if
negative so only actual elapsed blocks are charged; ensure the resulting
heightDelta is the correct integer type used by owed.MulInt64Mut (convert or
cast as needed) and use the clamped value in owed calculation and any subsequent
logic that decides overdraft state.
| store.Delete(key) | ||
|
|
||
| payments[i].State.State = etypes.StateClosed | ||
| if overdraft { | ||
| payments[i].State.State = etypes.StateOverdrawn | ||
| } | ||
|
|
||
| payments[i].State.Balance.Amount.Set(sdkmath.LegacyZeroDec()) | ||
| payments[i].State.Unsettled.Amount.Set(payments[i].State.Rate.Amount.MulInt64Mut(heightDelta)) | ||
|
|
||
| key = ekeeper.BuildPaymentsKey(payments[i].State.State, &payments[i].ID) | ||
| err = hooks.OnEscrowPaymentClosed(ctx, payments[i]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid mutating the stored payment rate while seeding unsettled debt.
On Line 167 we call MulInt64Mut directly on payments[i].State.Rate.Amount. That method mutates the rate in place, so after the upgrade every payment’s per-block rate is multiplied by heightDelta, breaking future settlements. Clone the rate (or work on a copy) before scaling and write only the derived value into Unsettled.
- payments[i].State.Unsettled.Amount.Set(payments[i].State.Rate.Amount.MulInt64Mut(heightDelta))
+ unsettled := payments[i].State.Rate.Amount
+ unsettled.MulInt64Mut(heightDelta)
+ payments[i].State.Unsettled.Amount = unsettled📝 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.
| store.Delete(key) | |
| payments[i].State.State = etypes.StateClosed | |
| if overdraft { | |
| payments[i].State.State = etypes.StateOverdrawn | |
| } | |
| payments[i].State.Balance.Amount.Set(sdkmath.LegacyZeroDec()) | |
| payments[i].State.Unsettled.Amount.Set(payments[i].State.Rate.Amount.MulInt64Mut(heightDelta)) | |
| key = ekeeper.BuildPaymentsKey(payments[i].State.State, &payments[i].ID) | |
| err = hooks.OnEscrowPaymentClosed(ctx, payments[i]) | |
| if err != nil { | |
| return err | |
| } | |
| store.Delete(key) | |
| payments[i].State.State = etypes.StateClosed | |
| if overdraft { | |
| payments[i].State.State = etypes.StateOverdrawn | |
| } | |
| payments[i].State.Balance.Amount.Set(sdkmath.LegacyZeroDec()) | |
| unsettled := payments[i].State.Rate.Amount | |
| unsettled.MulInt64Mut(heightDelta) | |
| payments[i].State.Unsettled.Amount.Set(unsettled) | |
| key = ekeeper.BuildPaymentsKey(payments[i].State.State, &payments[i].ID) | |
| err = hooks.OnEscrowPaymentClosed(ctx, payments[i]) | |
| if err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
In upgrades/software/v1.1.0/upgrade.go around lines 160 to 175 the code calls
MulInt64Mut on payments[i].State.Rate.Amount which mutates the stored per-block
rate; instead, create a copy of the Rate.Amount (or use the non-mutating
MulInt64) and perform the multiplication on that copy, then assign the result to
payments[i].State.Unsettled.Amount so the original payments[i].State.Rate.Amount
remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/market/keeper/keeper.go (1)
360-369: Clarify error handling logic for PaymentClose.The error handling appears inconsistent:
- Lines 360-362: Early return ensures
OnLeaseClosederrors propagate (good).- Line 363:
PaymentCloseis called, and its error is assigned to a shadowederrwithin the if block (line 363).- Lines 364-365: If
PaymentClosefails, the error is logged but execution continues.- Lines 366-368: The outer
errcheck is redundant because we already returned at line 361 iferrfromOnLeaseClosedwas non-nil. This check cannot catchPaymentCloseerrors since thaterrwas shadowed.If
PaymentCloseerrors should propagate, fix the shadowing:- if err := k.ekeeper.PaymentClose(ctx, lease.ID.ToEscrowPaymentID()); err != nil { + if err = k.ekeeper.PaymentClose(ctx, lease.ID.ToEscrowPaymentID()); err != nil { ctx.Logger().With("err", err).Info("error closing payment") + return err } - if err != nil { - return err - }If
PaymentCloseerrors should only be logged (non-blocking), remove the redundant check:if err := k.ekeeper.PaymentClose(ctx, lease.ID.ToEscrowPaymentID()); err != nil { ctx.Logger().With("err", err).Info("error closing payment") } - if err != nil { - return err - }
♻️ Duplicate comments (5)
make/test-upgrade.mk (1)
66-66: Test target still missing initialization dependency.This issue was previously flagged: the
testtarget no longer depends oninit, which means tests may run without proper setup (Cosmovisor installation and Akash initialization via$(AKASH_INIT)). This can lead to confusing test failures if prerequisites are not met.The previous review recommended either:
- Restoring the dependency:
test: init- Or documenting the external initialization requirement if it's intentionally handled elsewhere
x/escrow/keeper/keeper_settle_test.go (1)
104-104: Remove commented‑out dead code to satisfy lint and avoid confusionThe line
//initialBalance := sdkmath.LegacyMustNewDecFromStr(account.State.Deposits[0].Balance.Amount.String())is dead code, still missing a space after
//, and continues to triggercommentFormattingfromgolangci-lint. It should either be deleted or rewritten as a proper explanatory comment.Applying the earlier suggestion:
- //initialBalance := sdkmath.LegacyMustNewDecFromStr(account.State.Deposits[0].Balance.Amount.String())tests/upgrade/workers_test.go (1)
13-22: Document the no‑op v1.1.0 post‑upgrade workerRegistering the
"v1.1.0"post‑upgrade worker is correct, butpostUpgrade.Runis currently an empty no‑op. That’s fine if there are no invariants to check yet, but it should be explicit to future readers.Consider adding a brief TODO inside
Run:func (pu *postUpgrade) Run(ctx context.Context, t *testing.T, params uttypes.TestParams) { // TODO: add v1.1.0 upgrade invariant checks (or document why this worker is intentionally empty) }upgrades/software/v1.1.0/upgrade.go (2)
108-132: FixheightDeltato use a height difference, not a sum
heightDeltais currently computed as:heightDelta := ctx.BlockHeight() + val.State.SettledAtThis double‑counts blocks (current height plus previous
SettledAt) and will make every account appear dramatically overdue, likely forcing many otherwise‑healthy accounts into the overdraft path.It should be the elapsed blocks since the last settlement, clamped at zero:
- heightDelta := ctx.BlockHeight() + val.State.SettledAt + heightDelta := ctx.BlockHeight() - val.State.SettledAt + if heightDelta < 0 { + heightDelta = 0 + }Use this corrected
heightDeltafor theowedcalculation and all later usages so only the actual elapsed blocks are charged.
167-169: Avoid mutatingRate.Amountwhen seedingUnsettledHere:
payments[i].State.Balance.Amount.Set(sdkmath.LegacyZeroDec()) payments[i].State.Unsettled.Amount.Set(payments[i].State.Rate.Amount.MulInt64Mut(heightDelta))
MulInt64Mutmutatespayments[i].State.Rate.Amountin place, so after the upgrade each payment’s per‑block rate is multiplied byheightDelta. Future settlements will then chargerate * heightDeltaper block instead of the original rate, which is a serious behavior change.Compute
Unsettledon a copy of the rate instead:- payments[i].State.Balance.Amount.Set(sdkmath.LegacyZeroDec()) - payments[i].State.Unsettled.Amount.Set(payments[i].State.Rate.Amount.MulInt64Mut(heightDelta)) + payments[i].State.Balance.Amount.Set(sdkmath.LegacyZeroDec()) + + unsettled := payments[i].State.Rate.Amount + unsettled.MulInt64Mut(heightDelta) + payments[i].State.Unsettled.Amount.Set(unsettled)This preserves the original per‑block rate while still recording the accumulated unsettled amount.
🧹 Nitpick comments (6)
x/escrow/keeper/grpc_query_test.go (2)
28-51: Embedding*state.TestSuitealongside duplicated fields is slightly redundantEmbedding
*state.TestSuitewhile also carryingctx,app, and keeper fields works but introduces two sources of truth for the same state. Over time, this can drift if one is updated without the other. Consider either:
- relying on the embedded
TestSuitemethods/fields (Context(),App(), keepers, etc.), or- dropping the embedding and keeping only the explicit fields,
to keep the suite’s state centralized.
269-282: Bank keeper mocks increateEscrowAccountare correct but duplicated across testsStubbing
SendCoinsFromAccountToModule,SendCoinsFromModuleToAccount, andSendCoinsFromModuleToModuleto returnnilviaPrepareMocksis a good way to isolate escrow logic from bank I/O in these gRPC tests.Given that the same three expectations are used in multiple suites (and also in market tests), consider extracting a small helper, e.g.:
func prepareBankMocks(ts *state.TestSuite) { bkeeper := ts.BankKeeper() bkeeper. On("SendCoinsFromAccountToModule", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) bkeeper. On("SendCoinsFromModuleToAccount", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) bkeeper. On("SendCoinsFromModuleToModule", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) }and then calling
suite.PrepareMocks(prepareBankMocks)in each test that needs it.x/market/keeper/grpc_query_test.go (1)
420-432: Repeated BankKeeper mocks should be centralizedEach test configures the same three BankKeeper methods (
SendCoinsFromAccountToModule,SendCoinsFromModuleToAccount,SendCoinsFromModuleToModule) in aPrepareMocksblock. The logic is correct, but it’s duplicated in multiple places.Consider factoring this into a shared helper (in this file or a common test helper), e.g.:
func prepareBankMocks(ts *state.TestSuite) { bkeeper := ts.BankKeeper() bkeeper. On("SendCoinsFromAccountToModule", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) bkeeper. On("SendCoinsFromModuleToAccount", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) bkeeper. On("SendCoinsFromModuleToModule", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) }and then reusing it in all these tests with
suite.PrepareMocks(prepareBankMocks).Also applies to: 657-669, 894-906, 980-992, 1057-1069, 1145-1157
x/escrow/keeper/keeper_settle_test.go (1)
111-140: Expanded accounting checks are sound; consider asserting zero unsettled when not overdrawnThe new assertions around
totalTransferred,totalPayments, andtotalUnsettlednicely enforce:
account.State.Transferred[0].Amount == sum(cfg.transferred).account.State.Funds[0].Amount == balanceStart - (sum(Balance) + sum(Unsettled)).- In overdrawn cases, funds are negative and
funds + totalUnsettled == 0.Two small follow‑ups to tighten invariants and reduce noise:
- For non‑overdrawn cases, it’s worth explicitly asserting that there is no residual unsettled amount, to guard against scenarios where we might accidentally leave dust in
Unsettledwithout flaggingoverdrawn:if !overdrawn { assert.True(t, totalUnsettled.IsZero(), fmt.Sprintf("%s: unsettled must be zero when account is not overdrawn", tt.name)) }
- Instead of comparing
unsettledDiff.String()to a zero string, you can rely directly on the numeric predicate for clarity:unsettledDiff := account.State.Funds[0].Amount.Add(totalUnsettled) assert.True(t, unsettledDiff.IsZero(), fmt.Sprintf("%s: funds + unsettled must sum to zero when overdrawn", tt.name))This keeps the test focused on the numeric properties rather than string representations.
tests/upgrade/upgrade_test.go (1)
25-25: Upgrade test plumbing and proposal submission refactor look good, with a few assumptions to make explicitThe changes in this file hang together well:
- Importing
upgradetypesand usingupgradetypes.PlaninsideSoftwareUpgradeProposalis the right direction for v1 gov.- Adding
sourcesdirand threading it intopostUpgradeParams.SourceDirgives post‑upgrade workers access to the build tree, which is useful for richer checks.- Capturing
upgradeCacheand using it ascacheDirfor the proposal JSON keeps the file alongside validator state.- Extending validator env with
AKASH_PRUNING=nothingis sensible for upgrade/e2e runs to keep full history around.- The new JSON helpers (
baseAccount,moduleAccount,accountResp) and thequery auth module-account govcall correctly derive the authority address forMsgSoftwareUpgrade.submitUpgradeProposalnow:
- waits for the node to finish catching up,
- queries
gov params, parsesVotingPeriodas a duration,- computes
upgradeHeightascurrentHeight + votePeriod/(6s) + 10,- builds a
SoftwareUpgradeProposalmessage and wraps it inProposalMsg,- writes it to
<cacheDir>/upgrade-prop-<name>.json,- runs
tx gov submit-proposal <file>,- then discovers the proposal by
Title == upgradeNameand votesyesby ID.A couple of minor points you may want to tighten:
Tie
cacheDirto the commander’s home explicitlyRight now,
cacheDiris set to the homedir of the first validator (cfg.Validators[0]), whilecmdris bound to the validator named incfg.Work.Home. If those ever diverge in the config, the proposal file will live under one home while the commander’s AKASH_HOME/env points at another. The CLI only needs a readable absolute path, so this still works as long as everything shares a filesystem, but it’s less surprising to align them, e.g.:upgradeCache = homedir // where homedir belongs to cfg.Work.Homeor simply derive
cacheDirfrompostUpgradeParams.Home.Make the 6‑second block time assumption explicit
The computation:
upgradeHeight += int64(votePeriod/(6*time.Second)) + 10bakes in a 6s block time. That’s true for Akash today, but adding a
const blockTime = 6 * time.Second(or a comment) would make the assumption clear and easier to adjust if block time changes.Authority and gov JSON shape
The
accountResp/gov paramsunmarshalling assumes the current JSON layout ofquery auth module-account govandquery gov params. If either changes in a future SDK upgrade, these tests will fail early, which is acceptable, but worth keeping in mind when bumping Cosmos SDK.Overall, the refactor to JSON‑driven proposal submission via
tx gov submit-proposalis a solid improvement over hard‑coded CLI flags.Also applies to: 210-215, 232-240, 271-288, 349-357, 397-399, 452-499, 511-517, 642-652, 684-786, 792-803, 816-820
x/deployment/handler/handler_test.go (1)
443-443: Remove commented-out code.The commented line is unused and should be removed to keep the codebase clean. The
ownervariable is correctly derived at line 617 where it's actually needed.- //owner := sdk.MustAccAddressFromBech32(deployment.ID.Owner)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.github/workflows/tests.yaml(1 hunks)app/app.go(1 hunks)app/types/app.go(2 hunks)go.mod(1 hunks)make/test-upgrade.mk(3 hunks)meta.json(1 hunks)tests/upgrade/sdktypes.go(1 hunks)tests/upgrade/test-cases.json(3 hunks)tests/upgrade/test-config.json(1 hunks)tests/upgrade/types/types.go(1 hunks)tests/upgrade/upgrade_test.go(14 hunks)tests/upgrade/workers_test.go(1 hunks)testutil/state/suite.go(2 hunks)upgrades/software/v1.1.0/init.go(1 hunks)upgrades/software/v1.1.0/upgrade.go(1 hunks)upgrades/upgrades.go(1 hunks)x/deployment/handler/handler_test.go(14 hunks)x/deployment/keeper/grpc_query_test.go(8 hunks)x/escrow/genesis.go(1 hunks)x/escrow/keeper/grpc_query.go(3 hunks)x/escrow/keeper/grpc_query_test.go(4 hunks)x/escrow/keeper/keeper.go(23 hunks)x/escrow/keeper/keeper_settle_test.go(2 hunks)x/escrow/keeper/keeper_test.go(10 hunks)x/market/handler/handler_test.go(16 hunks)x/market/hooks/hooks.go(3 hunks)x/market/keeper/grpc_query_test.go(7 hunks)x/market/keeper/keeper.go(1 hunks)x/market/keeper/keeper_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- x/escrow/genesis.go
- x/market/keeper/keeper_test.go
- app/types/app.go
- testutil/state/suite.go
- .github/workflows/tests.yaml
- upgrades/software/v1.1.0/init.go
- app/app.go
- tests/upgrade/types/types.go
🧰 Additional context used
🧬 Code graph analysis (9)
x/deployment/keeper/grpc_query_test.go (2)
testutil/state/suite.go (1)
TestSuite(42-48)x/escrow/keeper/external.go (1)
BankKeeper(12-18)
x/escrow/keeper/grpc_query_test.go (1)
testutil/state/suite.go (1)
TestSuite(42-48)
tests/upgrade/workers_test.go (1)
tests/upgrade/types/types.go (1)
RegisterPostUpgradeWorker(38-44)
x/escrow/keeper/grpc_query.go (1)
x/escrow/keeper/key.go (2)
AccountPrefix(20-20)PaymentPrefix(21-21)
x/deployment/handler/handler_test.go (3)
testutil/cosmos/keepers.go (1)
BankKeeper(12-18)x/deployment/handler/keepers.go (1)
BankKeeper(38-40)x/escrow/keeper/external.go (1)
BankKeeper(12-18)
x/escrow/keeper/keeper.go (1)
x/escrow/keeper/key.go (2)
BuildPaymentsKey(50-56)ParsePaymentKey(122-160)
x/escrow/keeper/keeper_test.go (3)
testutil/state/suite.go (2)
TestSuite(42-48)SetupTestSuite(63-65)testutil/cosmos/keepers.go (1)
BankKeeper(12-18)x/escrow/keeper/external.go (1)
BankKeeper(12-18)
x/market/keeper/grpc_query_test.go (2)
testutil/state/suite.go (1)
TestSuite(42-48)x/deployment/handler/keepers.go (1)
BankKeeper(38-40)
tests/upgrade/upgrade_test.go (1)
tests/upgrade/sdktypes.go (2)
SoftwareUpgradeProposal(42-46)ProposalMsg(48-56)
🪛 golangci-lint (2.5.0)
tests/upgrade/sdktypes.go
[error] 13-13: type nodeStatus is unused
(unused)
[error] 20-20: type votingParams is unused
(unused)
[error] 24-24: type depositParams is unused
(unused)
[error] 28-28: type govParams is unused
(unused)
[error] 33-33: type proposalResp is unused
(unused)
[error] 38-38: type proposalsResp is unused
(unused)
x/deployment/handler/handler_test.go
[error] 443-443: commentFormatting: put a space between // and comment text
(gocritic)
x/escrow/keeper/keeper_test.go
[error] 20-20: type kTestSuite is unused
(unused)
x/market/handler/handler_test.go
[error] 1431-1431: func (*testSuite).createDeployment is unused
(unused)
x/escrow/keeper/keeper_settle_test.go
[error] 104-104: commentFormatting: put a space between // and comment text
(gocritic)
⏰ 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). (5)
- GitHub Check: lint-go
- GitHub Check: release-dry-run
- GitHub Check: sims
- GitHub Check: build-bins
- GitHub Check: coverage
🔇 Additional comments (33)
tests/upgrade/test-cases.json (2)
24-28: Inconsistency between AI summary and actual code changes.The AI-generated summary mentions inserting "empty object {}" placeholders, but the marked lines (24-28 and 174-178) contain unchanged migration entries ("from": "4", "to": "5" for auth, and "from": "3", "to": "4" for transfer). These appear to be existing entries, not new insertions, which conflicts with the summary's description.
Please clarify: Are these lines marked as changed due to formatting/whitespace modifications only, or are they substantive changes? If only formatting, they likely don't warrant being flagged as line changes in a code review.
Also applies to: 174-178
2-7: v1.1.0 test configuration is empty while metadata marks it as an active upgrade.The test-cases.json has v1.1.0 with no migrations defined, yet meta.json confirms v1.1.0 as an active upgrade path (
skipped: false, from v1.0.0). This inconsistency requires verification. While v1.0.0 defines comprehensive migrations for 16+ modules, v1.1.0 has none.Unable to locate upgrade handler implementation files or PR documentation in the codebase to determine what migrations (if any) v1.1.0 should include.
make/test-upgrade.mk (3)
79-79: LGTM - Enabling binary preparation during reset.Uncommenting the
binsinvocation ensures that binaries are properly fetched and prepared during the test reset process, which aligns with a complete reset workflow.
70-70: Based on my verification, the original review comment's concern is technically valid but depends on usage context.Key findings:
make/test-upgrade.mkuses$(AKASH_ROOT)on line 70 without verifying itmake/test-upgrade.mkhas NO include statements and is NOT included by the main Makefilemake/init.mk(which IS included by the main Makefile) contains the AKASH_ROOT verification:ifndef AKASH_ROOTwith an error messagemake/test-upgrade.mkalso uses many other undefined-in-file variables ($(AKASH_RUN),$(ROOT_DIR),$(GO_TEST),$(COSMOVISOR)) that suggest it relies on variables being initialized externallyAssessment:
If
make/test-upgrade.mkis invoked through normal main Makefile targets,init.mkis loaded first and AKASH_ROOT is verified. However, if invoked directly (e.g.,make -f make/test-upgrade.mk test), the check is bypassed. The file has an implicit dependency on AKASH_ROOT being set but provides no explicit verification.
24-24: Verify the incomplete CI integration for dynamic snapshot source determination.The change to
SNAPSHOT_SOURCE ?= mainnet(line 24) appears intentional and aligns with thesnapshot-sourcelogic inscript/upgrades.sh. However, the CI workflow has commented out the code that invokes this dynamic determination (.github/workflows/tests.yamllines 180, 182), causing the upgrade tests to always use the hardcoded mainnet default instead of dynamically selecting based on the git reference.Action required:
- Either uncomment lines 180-182 in
.github/workflows/tests.yamlto restore dynamic snapshot source selection, or- Document why mainnet is now the permanent default and confirm this doesn't break other workflows or development branches
- Verify mainnet snapshots are reliably available for all upgrade test scenarios
go.mod (1)
48-48: Verify compatibility of v0.1.6 escrow changes with this PR's escrow refactoring.v0.1.6 contains non-trivial modifications to
go/node/market/v1/escrow.go, specifically theLeaseIDFromPaymentIDfunction: the payment ID validation changed from checking 3-part splits to 5-part splits, and error handling was refactored. Since this PR also modifies escrow module logic (keeper hooks, payment withdrawal), ensure these changes integrate correctly and don't introduce validation mismatches.tests/upgrade/test-config.json (1)
1-10: LGTM! Simplified validator configuration for testing.The reduction to a single validator simplifies the upgrade test setup while maintaining necessary test coverage.
meta.json (1)
47-52: LGTM! New upgrade configuration added.The v1.1.0 upgrade entry is properly configured with the upgrade path from v1.0.0 (using binary v1.0.4).
x/market/handler/handler_test.go (4)
38-65: LGTM! Test suite setup enhanced for comprehensive flow testing.The addition of deployment and escrow handlers enables testing of complete end-to-end workflows involving multiple modules.
76-380: LGTM! Comprehensive end-to-end test with balance tracking.This test thoroughly validates the complete lifecycle including deployment creation, bidding, lease activation, overdrawn state, and recovery through additional deposits. The balance tracking via mock callbacks ensures proper fund flow verification.
382-618: LGTM! Comprehensive test coverage for different closure paths.These tests validate lease closure and bid closure flows respectively, ensuring proper handling of overdrawn states and recovery in both scenarios.
Also applies to: 620-856
876-888: LGTM! Existing tests updated with consistent mock patterns.The PrepareMocks usage ensures tests can execute with deterministic bank transfer behavior.
Also applies to: 913-925, 965-977, 999-1011, 1035-1047, 1063-1075, 1094-1106, 1186-1198, 1218-1230, 1247-1259, 1286-1298
x/deployment/keeper/grpc_query_test.go (2)
28-61: LGTM! Test suite enhanced with shared state access.Embedding
*state.TestSuiteenables access to the newPrepareMockshelper for deterministic bank transfer mocking.
66-78: LGTM! Consistent mock setup across test cases.The PrepareMocks blocks ensure deterministic behavior for bank transfers during deployment and escrow operations.
Also applies to: 156-168, 289-301, 496-508, 583-595
x/escrow/keeper/grpc_query.go (1)
250-264: LGTM! Helper function exported for external use.The function is now accessible outside the package. The logic remains unchanged, and all call sites have been properly updated.
tests/upgrade/sdktypes.go (1)
1-56: LGTM! Upgrade test scaffolding types added.These types provide structured handling for upgrade proposals and related data in tests. The unexported types flagged by static analysis are likely used for JSON unmarshaling in upgrade test flows, making the warnings false positives.
upgrades/upgrades.go (1)
1-6: LGTM! Upgrade package updated to v1.1.0.This change aligns with the new v1.1.0 upgrade entry added to meta.json and ensures the upgrade logic is properly registered.
x/escrow/keeper/grpc_query_test.go (1)
8-8: Mock import usage is appropriateUsing
github.com/stretchr/testify/mockhere is consistent with the newPrepareMocks-based test harness and looks correct.x/market/keeper/grpc_query_test.go (1)
9-9: Mock import integration is fineBringing in
github.com/stretchr/testify/mockmatches the new BankKeeper stubbing and is used appropriately below.upgrades/software/v1.1.0/upgrade.go (1)
244-466: Account payment scan and market/escrow hooks look consistent with existing patternsThe helper and hook methods:
accountPayments– iterating prefixed payment keys per state, unmarshalling intoetypes.Payment.OnEscrowAccountClosed– resolving deployment ID, closing active deployments and groups, and delegating toOnGroupClosed.OnGroupClosed– closing active orders and iterating bids to close them (and any associated leases) via the market keeper.OnEscrowPaymentClosed/OnBidClosed/updateBid– wiring payment closure back to bids, orders, leases, and emittingEventBidClosedwhile maintaining state/reverse keys.all mirror the usual x/deployment and x/market keeper patterns: they guard on current states, short‑circuit cleanly when related objects are missing or already closed, and maintain both forward and reverse store keys.
From this file’s context, the store mutations and event emissions are coherent and should integrate correctly with the existing module invariants.
x/escrow/keeper/keeper.go (8)
27-28: LGTM: Error propagation enabled for hooks.The updated hook signatures allow proper error handling during account and payment closure callbacks, which is essential for robust state management.
438-438: Decimal truncation during deposit aligns with integer coin semantics.The
.TruncateInt()call correctly convertsDecCointoCoinfor the bank transfer, as the Cosmos SDK bank module only handles integer amounts. This prevents fractional token units from causing issues.Note: The PR title mentions "truncate dec part during payment withdraw," but truncation is also necessary here during deposit to comply with SDK constraints.
461-465: Overdraft accounts freeze settlement time by design.The code intentionally does not advance
SettledAtfor overdrawn accounts, preventing further payment accrual. This design assumes that associated resources (e.g., deployments) are closed when overdraft occurs.Ensure this behavior is documented in module specs, as it's a critical invariant for account lifecycle management.
917-948: LGTM: accountPayments now supports multi-state queries.The refactored
accountPaymentsmethod efficiently retrieves payments across multiple states (e.g., Open and Overdrawn) in a single call, reducing redundant store iterations.
956-956: Key fix: Decimal truncation during payment withdrawal.The
TruncateInt()call ensures fractional uakt are dropped when converting payment balance to withdrawable coins, preventing residual fractional amounts from remaining after account closure. This directly addresses the PR objective.This is the primary fix described in the PR title: "truncate dec part during payment withdraw."
1090-1103: LGTM: Transfer calculation handles overdrawn and open payments.The logic correctly differentiates between:
- Overdrawn payments: transfers accumulated unsettled amounts
- Open payments: calculates new transfers based on rate × block delta
455-457: Early return behavior is correct and requires no changes.When
accountSettledetects funds are already negative (lines 455-457), it immediately returnsnil, true, nil. This is an intentional early exit for terminal overdraft state, not a bug.The design is sound because:
Correct early exit: When funds are already negative at the start of settlement, no further settlement is possible—all prior settlements have already occurred. Returning nil payments reflects this correctly.
Aligned with callers: In
AccountDeposit(line 352), when the early return occurs, the loop over payments (line 360) simply doesn't execute, and the account correctly remainsStateOverdrawnbecauseod=trueprevents the state transition.Reflects intended semantics: The comment at lines 459-460 confirms this behavior: "overdrawn account does not update settledAt, as associated objects like deployment are closed." The early return is the mechanism enforcing this.
431-433: The review comment is based on a misunderstanding of the code flow.The reset to zero at lines 431-433 is not the final state—it's an initialization step during deposit accumulation. The comment within the code explicitly states "later down in this function it will trigger account settlement and recalculate the owed balance," and inspection confirms this happens immediately after at line 351-352.
Additionally, tests in
keeper_settle_test.go(lines 134-140) validate that negative balances are intentional and properly tracked: the test asserts balances ARE negative when overdrawn and verifies thatnegative_balance + total_unsettled = zero, confirming no debt is lost. The settlement logic at line 455 explicitly handles negative balances without error. No data loss or accounting issues occur.Likely an incorrect or invalid review comment.
x/deployment/handler/handler_test.go (2)
172-178: LGTM: PrepareMocks enables granular test mock configuration.The new
PrepareMockspattern allows each test to configure only the specific bank transfer expectations it needs, improving test clarity and reducing false positive mock matches.This pattern is consistently applied throughout the test suite and aligns with the refactored escrow keeper that now validates fund transfers at each operation.
619-630: LGTM: Comprehensive validation of multi-depositor fund returns.The test correctly mocks fund returns to all four depositors (owner, granter, random depositor, and provider) during deployment closure, ensuring proper escrow accounting across multiple deposit sources.
x/escrow/keeper/keeper_test.go (2)
24-85: LGTM: Comprehensive account settlement test.The new test validates the complete settlement flow including:
- Account creation with deposits
- Payment creation and rate accrual
- Settlement on account closure with proper fund distribution
- Take rate fees sent to community pool
- Final state verification
This test provides excellent coverage for the PR's core functionality around decimal truncation during withdrawals.
258-388: LGTM: Thorough overdraft scenario testing.The refactored test comprehensively validates:
- Overdraft detection when payments exceed deposits
- Proper state transitions to
StateOverdrawn- Unsettled amount tracking
- Multiple account close attempts (idempotency)
- Overdraft recovery through deposits with proper fund distribution
This test ensures the new error-aware settlement logic handles edge cases correctly.
x/market/hooks/hooks.go (1)
14-15: LGTM: Hook error propagation implemented correctly.The interface methods now return errors, enabling proper error handling during escrow callbacks. All implementations correctly:
- Propagate errors from keeper operations
- Return
nilfor not-found or inactive entities (graceful handling)- Return specific errors for missing required entities
This change was previously reviewed and addressed per past comments. The implementation properly handles all error paths.
Also applies to: 30-117
| type kTestSuite struct { | ||
| *state.TestSuite | ||
| } |
There was a problem hiding this comment.
Remove unused type definition.
The kTestSuite type is defined but never used in any tests. All tests directly use state.SetupTestSuite(t) instead.
-type kTestSuite struct {
- *state.TestSuite
-}
-📝 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.
| type kTestSuite struct { | |
| *state.TestSuite | |
| } |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 20-20: type kTestSuite is unused
(unused)
🤖 Prompt for AI Agents
In x/escrow/keeper/keeper_test.go around lines 20 to 22, the type definition
`kTestSuite` is unused; delete that entire type block from the file and run `go
vet`/`go test` to ensure no references remain; also remove any now-unused
imports introduced only for this type and run `gofmt`/`goimports` to clean up
formatting and import list.
x/market/handler/handler_test.go
Outdated
| require.True(t, found) | ||
| require.Equal(t, dv1.DeploymentClosed, depl.State) | ||
|
|
||
| // should ont be able to close escrow account in overdrawn state |
There was a problem hiding this comment.
Fix typo in comment.
- // should ont be able to close escrow account in overdrawn state
+ // should not be able to close escrow account in overdrawn state📝 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.
| // should ont be able to close escrow account in overdrawn state | |
| // should not be able to close escrow account in overdrawn state |
🤖 Prompt for AI Agents
In x/market/handler/handler_test.go around line 310, the inline comment contains
a typo ("ont" and "in overdrawn state"); update the comment to read clearly,
e.g. "should not be able to close escrow account in overdrawn state", correcting
"ont" to "not" and fixing surrounding grammar for clarity.
| func (st *testSuite) createDeployment() (dv1.Deployment, dtypes.Groups) { | ||
| st.t.Helper() | ||
|
|
||
| deployment := testutil.Deployment(st.t) | ||
| group := testutil.DeploymentGroup(st.t, deployment.ID, 0) | ||
| group.GroupSpec.Resources = dtypes.ResourceUnits{ | ||
| { | ||
| Resources: testutil.ResourceUnits(st.t), | ||
| Count: 1, | ||
| Price: testutil.AkashDecCoinRandom(st.t), | ||
| }, | ||
| } | ||
| groups := dtypes.Groups{ | ||
| group, | ||
| } | ||
|
|
||
| for i := range groups { | ||
| groups[i].State = dtypes.GroupOpen | ||
| } | ||
|
|
||
| return deployment, groups | ||
| } |
There was a problem hiding this comment.
Remove unused helper function.
Static analysis indicates this function is unused. Consider removing it or using it in the test suite if it was intended for testing purposes.
-func (st *testSuite) createDeployment() (dv1.Deployment, dtypes.Groups) {
- st.t.Helper()
-
- deployment := testutil.Deployment(st.t)
- group := testutil.DeploymentGroup(st.t, deployment.ID, 0)
- group.GroupSpec.Resources = dtypes.ResourceUnits{
- {
- Resources: testutil.ResourceUnits(st.t),
- Count: 1,
- Price: testutil.AkashDecCoinRandom(st.t),
- },
- }
- groups := dtypes.Groups{
- group,
- }
-
- for i := range groups {
- groups[i].State = dtypes.GroupOpen
- }
-
- return deployment, groups
-}📝 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 (st *testSuite) createDeployment() (dv1.Deployment, dtypes.Groups) { | |
| st.t.Helper() | |
| deployment := testutil.Deployment(st.t) | |
| group := testutil.DeploymentGroup(st.t, deployment.ID, 0) | |
| group.GroupSpec.Resources = dtypes.ResourceUnits{ | |
| { | |
| Resources: testutil.ResourceUnits(st.t), | |
| Count: 1, | |
| Price: testutil.AkashDecCoinRandom(st.t), | |
| }, | |
| } | |
| groups := dtypes.Groups{ | |
| group, | |
| } | |
| for i := range groups { | |
| groups[i].State = dtypes.GroupOpen | |
| } | |
| return deployment, groups | |
| } |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 1431-1431: func (*testSuite).createDeployment is unused
(unused)
🤖 Prompt for AI Agents
In x/market/handler/handler_test.go around lines 1431 to 1452, the helper
function createDeployment is defined but not referenced anywhere; either remove
this unused function to clean up dead code or update the test suite to call it
where appropriate (e.g., replace duplicated deployment/group setup in tests with
createDeployment or add a new test that uses it). After making the change, run
`go test ./...` and static analysis (go vet/golangci-lint) to ensure no
remaining unused symbols or test regressions.
fixes issue when fractional of uakt remain in the balance after account is closed. refactor escrow testsuite to ensure funds are tracked during account operations and match expected values Signed-off-by: Artur Troian <troian@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/escrow/keeper/grpc_query.go (1)
250-264: Add documentation comment for the exported function.The function is now exported and is used in
upgrades/software/v1.1.0/upgrade.go, confirming that exporting it is necessary. However, exported functions must have documentation comments per Go conventions.Apply this diff to add the documentation:
+// BuildSearchPrefix constructs a search prefix for querying accounts or payments +// by combining the given prefix (e.g., AccountPrefix or PaymentPrefix) with optional +// state and xid filters. Returns the combined prefix as a byte slice. func BuildSearchPrefix(prefix []byte, state string, xid string) []byte {
♻️ Duplicate comments (5)
upgrades/software/v1.1.0/upgrade.go (2)
108-108: Critical: Fix the height delta calculation.Line 108 adds
val.State.SettledAtto the current block height, which double-counts elapsed blocks. This was flagged in a previous review and should use subtraction instead.See past review comment on lines 108-133 for detailed explanation and fix.
168-168: Critical: Avoid mutating the stored payment rate.Line 168 calls
MulInt64Mutdirectly onpayments[i].State.Rate.Amount, which mutates the stored per-block rate. This was flagged in a previous review and will break future settlements.See past review comment on lines 160-175 for detailed explanation and fix.
tests/upgrade/workers_test.go (1)
12-22: Document why the v1.1.0 post-upgrade worker is a no-op
postUpgrade.Runis currently empty, so the v1.1.0 post-upgrade path performs no checks. To avoid this silently remaining a no-op, add a brief TODO explaining what invariants should eventually be asserted (or why this worker is intentionally empty).Example:
func (pu *postUpgrade) Run(ctx context.Context, t *testing.T, params uttypes.TestParams) { - + // TODO: add v1.1.0 upgrade invariant checks when upgrade behavior is finalized }x/escrow/keeper/keeper_test.go (1)
20-22: Remove unusedkTestSuitewrapper type
kTestSuiteis defined but never used; all tests callstate.SetupTestSuite(t)directly. This is also reported by golangci-lint.You can drop the wrapper entirely:
-type kTestSuite struct { - *state.TestSuite -} -x/market/handler/handler_test.go (1)
1431-1452: Remove unusedcreateDeploymenthelper in market handler tests
func (st *testSuite) createDeployment()is not referenced anywhere in this file, and golangci-lint reports it as unused. Since an equivalent helper already exists in the deployment handler tests, it’s safe to delete this duplicate here.-func (st *testSuite) createDeployment() (dv1.Deployment, dtypes.Groups) { - st.t.Helper() - - deployment := testutil.Deployment(st.t) - group := testutil.DeploymentGroup(st.t, deployment.ID, 0) - group.GroupSpec.Resources = dtypes.ResourceUnits{ - { - Resources: testutil.ResourceUnits(st.t), - Count: 1, - Price: testutil.AkashDecCoinRandom(st.t), - }, - } - groups := dtypes.Groups{ - group, - } - - for i := range groups { - groups[i].State = dtypes.GroupOpen - } - - return deployment, groups -}
🧹 Nitpick comments (6)
x/market/keeper/keeper.go (1)
351-369: Early return onOnLeaseClosederror is good; trailingerrcheck is now dead codeThe new early return after
OnLeaseClosed(lines 359-362) makes sense and avoids callingPaymentClosewhen lease closure fails. Given this, and thatOnBidClosedis already guarded above, theerrat lines 366-368 can no longer be non‑nil at that point (theerrinsidePaymentCloseis a separate, inner-scoped variable). The finalif err != nil { return err }is therefore redundant and slightly confusing.Consider removing it for clarity:
- if err != nil { - return err - }x/deployment/keeper/grpc_query_test.go (1)
66-78: Consider extracting repeated mock setup into a helper.The same three bank keeper mocks are configured identically in five places. Consider extracting this pattern into a helper method like
prepareBankKeeperMocks(ts *state.TestSuite)to reduce duplication and improve maintainability.For example:
func prepareBankKeeperMocks(ts *state.TestSuite) { bkeeper := ts.BankKeeper() bkeeper.On("SendCoinsFromAccountToModule", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) bkeeper.On("SendCoinsFromModuleToAccount", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) bkeeper.On("SendCoinsFromModuleToModule", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) }Then in each test:
suite.PrepareMocks(prepareBankKeeperMocks)Also applies to: 156-168, 289-301, 496-508, 583-595
x/market/handler/handler_test.go (1)
362-365: Align comments with final “closed” escrow/payment stateBoth places you comment that the escrow account and payment are “expected to be in overdrawn state”, but the assertions check for
etypes.StateClosed. To avoid confusion for future readers, update the comments to reflect the closed/settled state:- // both escrow account and payment are expected to be in overdrawn state + // after settling the overdraft, both escrow account and payment are expected to be closed(and similarly for the second occurrence)
Also applies to: 612-615
tests/upgrade/upgrade_test.go (3)
349-355: Document why the first validator's home directory is used as cache.The code sets
upgradeCacheto the first validator's home directory. Consider adding a comment explaining this choice, as it creates an implicit dependency on the validator ordering.
720-741: LGTM: Dynamic upgrade height calculation.The upgrade height calculation now uses the voting period duration and assumes a 6-second block time. This is more flexible than a hardcoded height.
Note: The calculation assumes
votePeriod / (6 * time.Second)fits in int64. For typical governance periods (days/weeks), this is safe, but consider adding a comment documenting the 6-second block time assumption.
758-766: Consider more descriptive proposal title and summary.Both
TitleandSummaryare set tol.upgradeName. While this works, consider whether a more descriptive summary would be beneficial for proposal tracking, especially if multiple proposals for the same upgrade might exist.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.github/workflows/tests.yaml(1 hunks)app/app.go(1 hunks)app/types/app.go(2 hunks)go.mod(1 hunks)make/test-upgrade.mk(3 hunks)meta.json(1 hunks)tests/upgrade/sdktypes.go(1 hunks)tests/upgrade/test-cases.json(3 hunks)tests/upgrade/test-config.json(1 hunks)tests/upgrade/types/types.go(1 hunks)tests/upgrade/upgrade_test.go(14 hunks)tests/upgrade/workers_test.go(1 hunks)testutil/state/suite.go(2 hunks)upgrades/software/v1.1.0/init.go(1 hunks)upgrades/software/v1.1.0/upgrade.go(1 hunks)upgrades/upgrades.go(1 hunks)x/deployment/handler/handler_test.go(14 hunks)x/deployment/keeper/grpc_query_test.go(8 hunks)x/escrow/genesis.go(1 hunks)x/escrow/keeper/grpc_query.go(3 hunks)x/escrow/keeper/grpc_query_test.go(4 hunks)x/escrow/keeper/keeper.go(23 hunks)x/escrow/keeper/keeper_settle_test.go(2 hunks)x/escrow/keeper/keeper_test.go(10 hunks)x/market/handler/handler_test.go(16 hunks)x/market/hooks/hooks.go(3 hunks)x/market/keeper/grpc_query_test.go(7 hunks)x/market/keeper/keeper.go(1 hunks)x/market/keeper/keeper_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- app/app.go
- go.mod
- tests/upgrade/test-config.json
- tests/upgrade/types/types.go
- tests/upgrade/test-cases.json
- make/test-upgrade.mk
- x/market/keeper/keeper_test.go
- upgrades/software/v1.1.0/init.go
- x/escrow/keeper/grpc_query_test.go
- app/types/app.go
- x/escrow/genesis.go
- x/market/keeper/grpc_query_test.go
- meta.json
🧰 Additional context used
🧬 Code graph analysis (8)
x/escrow/keeper/grpc_query.go (1)
x/escrow/keeper/key.go (2)
AccountPrefix(20-20)PaymentPrefix(21-21)
x/escrow/keeper/keeper_test.go (4)
testutil/state/suite.go (2)
TestSuite(42-48)SetupTestSuite(63-65)testutil/cosmos/keepers.go (1)
BankKeeper(12-18)x/escrow/keeper/external.go (1)
BankKeeper(12-18)x/market/keeper/external.go (1)
EscrowKeeper(9-14)
x/market/handler/handler_test.go (3)
x/deployment/handler/handler.go (1)
NewHandler(14-41)x/market/keeper/external.go (1)
EscrowKeeper(9-14)x/market/alias.go (1)
ModuleName(13-13)
upgrades/software/v1.1.0/upgrade.go (1)
x/escrow/keeper/key.go (5)
AccountPrefix(20-20)ParseAccountKey(71-120)BuildAccountsKey(27-33)BuildPaymentsKey(50-56)ParsePaymentKey(122-160)
x/deployment/keeper/grpc_query_test.go (3)
testutil/state/suite.go (1)
TestSuite(42-48)x/escrow/keeper/external.go (1)
BankKeeper(12-18)x/market/handler/keepers.go (1)
BankKeeper(56-59)
tests/upgrade/workers_test.go (1)
tests/upgrade/types/types.go (1)
RegisterPostUpgradeWorker(38-44)
x/deployment/handler/handler_test.go (2)
testutil/cosmos/keepers.go (1)
BankKeeper(12-18)x/escrow/keeper/external.go (1)
BankKeeper(12-18)
tests/upgrade/upgrade_test.go (1)
tests/upgrade/sdktypes.go (2)
SoftwareUpgradeProposal(42-46)ProposalMsg(48-56)
🪛 golangci-lint (2.5.0)
x/escrow/keeper/keeper_test.go
[error] 20-20: type kTestSuite is unused
(unused)
x/market/handler/handler_test.go
[error] 1431-1431: func (*testSuite).createDeployment is unused
(unused)
tests/upgrade/sdktypes.go
[error] 13-13: type nodeStatus is unused
(unused)
[error] 20-20: type votingParams is unused
(unused)
[error] 24-24: type depositParams is unused
(unused)
[error] 28-28: type govParams is unused
(unused)
[error] 33-33: type proposalResp is unused
(unused)
[error] 38-38: type proposalsResp is unused
(unused)
x/deployment/handler/handler_test.go
[error] 443-443: commentFormatting: put a space between // and comment text
(gocritic)
⏰ 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). (7)
- GitHub Check: network-upgrade
- GitHub Check: lint-go
- GitHub Check: release-dry-run
- GitHub Check: sims
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: build-bins
🔇 Additional comments (24)
x/escrow/keeper/grpc_query.go (2)
84-84: LGTM: Call site correctly updated to use exported function.The call to
BuildSearchPrefixwithAccountPrefixis correct and necessary after the function was exported.
196-196: LGTM: Call site correctly updated to use exported function.The call to
BuildSearchPrefixwithPaymentPrefixis correct and necessary after the function was exported.testutil/state/suite.go (1)
165-167: LGTM! Helpful test harness improvement.The
PrepareMockshelper enables tests to customize mocks after suite setup, improving test isolation and clarity.upgrades/upgrades.go (1)
5-5: LGTM! Standard upgrade version bump.The import correctly switches to the v1.1.0 upgrade implementation.
x/market/hooks/hooks.go (1)
14-15: LGTM! Proper error propagation added to hook methods.The hook interface methods now return errors, enabling proper error handling during escrow account and payment closure. While this is a breaking change, it's necessary for robust error handling and was properly addressed in a previous review.
Based on learnings, breaking changes to public interfaces should be carefully coordinated.
Also applies to: 30-117
x/escrow/keeper/keeper.go (3)
27-28: LGTM! Hook signatures updated for error propagation.The
AccountHookandPaymentHooksignatures now return errors, andSavePaymentalso returns an error. This aligns with the hook interface changes inx/market/hooks/hooks.goand enables proper error handling.Also applies to: 48-48
1101-1101: Key fix: Decimal truncation during settlement.Line 1101 uses
TruncateDec()to truncate the decimal part when calculating transfers during settlement. This appears to be the core fix mentioned in the PR objectives for preventing fractional uakt from remaining in balances after account closure.
431-438: Verified: negative balance reset logic is intentional and safe.The reset to zero at lines 431-432 does not forgive debt. When a balance is negative, it indicates an overdrawn account. The logic resets it to zero, then immediately adds the new deposit (line 442). Settlement is later triggered and recalculates all unsettled amounts based on the current deposit and balance state. The test at keeper_settle_test.go lines 137-138 confirms the invariant: when overdrawn,
account.State.Funds[0].Amount + totalUnsettled == 0, proving that negative balances are properly tracked in theUnsettledfield rather than being forgiven.x/escrow/keeper/keeper_settle_test.go (1)
110-139: LGTM! Comprehensive test coverage for settlement accounting.The expanded test assertions thoroughly validate transfers, payments, unsettled amounts, and expected remaining balances. This provides strong coverage for the settlement logic refactoring.
upgrades/software/v1.1.0/upgrade.go (1)
102-104: No issues—hardcoding is intentional for stable payment processing.The IBC denomination is axlUSDC (Axelar-bridged USDC), used for Stable Payments on Akash, including deployment deposits and settlement. The check is appropriately scoped to handle this specific stable payment token, not a general-purpose upgrade handler. No changes needed.
.github/workflows/tests.yaml (1)
173-176: The new environment variables are not used downstream; no verification needed.The review comment assumes downstream scripts reference these variables, but comprehensive codebase analysis reveals neither
DOCKER_IDU/DOCKER_IDGnor the previousDOCKER_USER/DOCKER_GROUPare referenced anywhere—no shell scripts, Docker commands, workflows, or other tools consume them. The variables appear to be unused/dead code, so renaming them poses no breaking change.Likely an incorrect or invalid review comment.
x/escrow/keeper/keeper_test.go (1)
24-255: Escrow settlement and overdraft test flows look consistentThe new account/payment settlement and overdraft scenarios (including 2% take rate, 1000 uakt deposits, 10 uakt/block rate, overdraft of 50 uakt, and subsequent payoff/closure) are internally consistent with the comments and bank mocks; they exercise the key edge cases around withdrawals, overdraft state, and final closure.
Also applies to: 258-388
x/market/handler/handler_test.go (1)
38-65: End-to-end market + escrow full-flow tests look solidThe new full-flow tests for closing deployments, leases, and bids (with overdraft, later deposits, and settlement via distribution/escrow) are well-structured: the local
balances/escrowBalance/distrBalancebookkeeping matches the BankKeeper mocks, and the final state assertions for bids, leases, deployments, and escrow accounts/payments line up with the described scenarios.Also applies to: 76-380, 382-618, 620-856
tests/upgrade/upgrade_test.go (11)
234-234: LGTM: sourcesdir flag handling is consistent.The new
sourcesdirflag is properly validated, trimmed, and used consistently with the existingworkdirpattern.Also applies to: 271-271, 287-287
398-398: LGTM: SourceDir parameter properly propagated.The SourceDir is correctly set from the sourcesdir flag and passed to post-upgrade test parameters.
498-498: LGTM: Pruning set to nothing for upgrade tests.Setting
AKASH_PRUNING=nothingis appropriate for upgrade tests, ensuring all blocks are retained for verification and replay.
684-694: LGTM: Module account query for new governance flow.The code correctly queries the gov module account to obtain the authority address required for the new proposal submission flow. Error handling is appropriately implemented.
773-777: LGTM: Proposal file creation.The proposal JSON is correctly written to a file in the cache directory for submission. The 0644 permissions are appropriate for a read-only proposal file.
779-782: LGTM: Enhanced logging for debugging.The updated log message provides useful context including vote period, current height, and calculated upgrade height, which aids in debugging timing-related issues.
784-784: LGTM: File-based proposal submission.The command correctly uses the generated proposal file, aligning with the new governance module's file-based submission flow.
743-751: No action required—type string verified as correct.The
Typefield is correctly set to"/cosmos.upgrade.v1beta1.MsgSoftwareUpgrade"per Cosmos SDK v0.50 documentation.
642-652: No action required—account response structure is correct.The Cosmos SDK
query auth module-accountresponse has the structure:account→base_account→address, which matches your Go struct hierarchy (accountResp.Account.BaseAccount.Address). The structs will successfully unmarshal the JSON response. The implementation captures only the fields needed and ignores extra API fields (like@type,name,permissions), which is standard Go practice.
818-820: No issues found.The code correctly accesses
proposals.Proposals[i].Titleandproposals.Proposals[i].ID. The variableproposalsis explicitly typed asproposalsResp(line 811) and unmarshaled from the JSON response. The struct definitions confirm thatproposalResphas bothIDandTitlefields with appropriate JSON tags (json:"id"andjson:"title"), which are standard Cosmos SDK naming conventions for proposal responses.
25-25: ****The import
upgradetypes "cosmossdk.io/x/upgrade/types"is correct and compatible. The package was extracted to a standalone module with imports moved to cosmossdk.io/x/upgrade, and v0.2.0 specified in go.mod is already in use throughout the codebase (app/upgrades.go, app/testnet.go, upgrades/software/). cosmossdk.io/x/upgrade v0.2.0 is used as a dependency in recent releases like ibc-go v10.2.0, confirming stability. No changes needed.
| type nodeStatus struct { | ||
| SyncInfo struct { | ||
| LatestBlockHeight string `json:"latest_block_height"` | ||
| CatchingUp bool `json:"catching_up"` | ||
| } `json:"sync_info"` | ||
| } | ||
|
|
||
| type votingParams struct { | ||
| VotingPeriod string `json:"voting_period"` | ||
| } | ||
|
|
||
| type depositParams struct { | ||
| MinDeposit sdk.Coins `json:"min_deposit"` | ||
| } | ||
|
|
||
| type govParams struct { | ||
| VotingParams votingParams `json:"voting_params"` | ||
| DepositParams depositParams `json:"deposit_params"` | ||
| } | ||
|
|
||
| type proposalResp struct { | ||
| ID string `json:"id"` | ||
| Title string `json:"title"` | ||
| } | ||
|
|
||
| type proposalsResp struct { | ||
| Proposals []proposalResp `json:"proposals"` | ||
| } |
There was a problem hiding this comment.
Clean up unused internal types flagged by golangci-lint
nodeStatus, votingParams, depositParams, govParams, proposalResp, and proposalsResp are defined but unused, and golangci-lint reports them as errors. Either wire them into tests or remove them for now to keep the test package clean.
If they’re not needed yet, you can safely delete them:
-type nodeStatus struct {
- SyncInfo struct {
- LatestBlockHeight string `json:"latest_block_height"`
- CatchingUp bool `json:"catching_up"`
- } `json:"sync_info"`
-}
-
-type votingParams struct {
- VotingPeriod string `json:"voting_period"`
-}
-
-type depositParams struct {
- MinDeposit sdk.Coins `json:"min_deposit"`
-}
-
-type govParams struct {
- VotingParams votingParams `json:"voting_params"`
- DepositParams depositParams `json:"deposit_params"`
-}
-
-type proposalResp struct {
- ID string `json:"id"`
- Title string `json:"title"`
-}
-
-type proposalsResp struct {
- Proposals []proposalResp `json:"proposals"`
-}📝 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.
| type nodeStatus struct { | |
| SyncInfo struct { | |
| LatestBlockHeight string `json:"latest_block_height"` | |
| CatchingUp bool `json:"catching_up"` | |
| } `json:"sync_info"` | |
| } | |
| type votingParams struct { | |
| VotingPeriod string `json:"voting_period"` | |
| } | |
| type depositParams struct { | |
| MinDeposit sdk.Coins `json:"min_deposit"` | |
| } | |
| type govParams struct { | |
| VotingParams votingParams `json:"voting_params"` | |
| DepositParams depositParams `json:"deposit_params"` | |
| } | |
| type proposalResp struct { | |
| ID string `json:"id"` | |
| Title string `json:"title"` | |
| } | |
| type proposalsResp struct { | |
| Proposals []proposalResp `json:"proposals"` | |
| } |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 13-13: type nodeStatus is unused
(unused)
[error] 20-20: type votingParams is unused
(unused)
[error] 24-24: type depositParams is unused
(unused)
[error] 28-28: type govParams is unused
(unused)
[error] 33-33: type proposalResp is unused
(unused)
[error] 38-38: type proposalsResp is unused
(unused)
🤖 Prompt for AI Agents
In tests/upgrade/sdktypes.go around lines 13 to 40, several types (nodeStatus,
votingParams, depositParams, govParams, proposalResp, proposalsResp) are
declared but unused and causing golangci-lint errors; remove these unused type
definitions from the file (or alternatively, if they are intended for future
use, reference them in tests to avoid lint failures) — the quick fix is to
delete the whole block of those type declarations so the test package no longer
contains unused internal types.
| //tm := time.NewTimer(30 * time.Second) | ||
| //select { | ||
| //case <-l.ctx.Done(): | ||
| // if !tm.Stop() { | ||
| // <-tm.C | ||
| // } | ||
| // err = l.ctx.Err() | ||
| // return err | ||
| //case <-tm.C: | ||
| //} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
The commented timer code (lines 696-705) should be removed rather than left as dead code. If this wait was intentionally removed, the commented code serves no purpose and adds clutter.
Apply this diff to remove the commented code:
- //tm := time.NewTimer(30 * time.Second)
- //select {
- //case <-l.ctx.Done():
- // if !tm.Stop() {
- // <-tm.C
- // }
- // err = l.ctx.Err()
- // return err
- //case <-tm.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.
| //tm := time.NewTimer(30 * time.Second) | |
| //select { | |
| //case <-l.ctx.Done(): | |
| // if !tm.Stop() { | |
| // <-tm.C | |
| // } | |
| // err = l.ctx.Err() | |
| // return err | |
| //case <-tm.C: | |
| //} |
🤖 Prompt for AI Agents
In tests/upgrade/upgrade_test.go around lines 696 to 705, remove the block of
commented-out timer/select code (the 10 lines starting with "//tm :=
time.NewTimer(30 * time.Second)" through the matching commented select/cases)
because it is dead/commented code; simply delete those commented lines so the
file contains no leftover commented timer logic and run `go vet`/`go test` to
ensure no formatting or build issues.
| // give it two blocks to make sure a proposal has been commited | ||
| tmctx, cancel := context.WithTimeout(l.ctx, 18*time.Second) |
There was a problem hiding this comment.
Fix comment-code inconsistency.
The comment states "give it two blocks" but the timeout is 18 seconds, which equals 3 blocks at 6 seconds per block. Either update the comment to "three blocks" or change the timeout to 12 seconds for two blocks.
Apply this diff to fix the comment:
- // give it two blocks to make sure a proposal has been commited
+ // give it three blocks to make sure a proposal has been commited
tmctx, cancel := context.WithTimeout(l.ctx, 18*time.Second)📝 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.
| // give it two blocks to make sure a proposal has been commited | |
| tmctx, cancel := context.WithTimeout(l.ctx, 18*time.Second) | |
| // give it three blocks to make sure a proposal has been commited | |
| tmctx, cancel := context.WithTimeout(l.ctx, 18*time.Second) |
🤖 Prompt for AI Agents
In tests/upgrade/upgrade_test.go around lines 792-793, the comment says "give it
two blocks" but the timeout is set to 18 seconds (3 blocks at 6s each); update
the comment to "give it three blocks" to match the 18s timeout, or alternatively
change the timeout to 12*time.Second to keep the "two blocks" comment—pick one
and make the code and comment consistent.
| //owner := sdk.MustAccAddressFromBech32(deployment.ID.Owner) | ||
| suite.PrepareMocks(func(ts *state.TestSuite) { | ||
| bkeeper := ts.BankKeeper() | ||
| bkeeper. | ||
| On("SendCoinsFromAccountToModule", mock.Anything, mock.Anything, emodule.ModuleName, sdk.Coins{msg.Deposit.Amount}). | ||
| Return(nil).Once() | ||
| }) |
There was a problem hiding this comment.
Fix comment formatting to satisfy gocritic
There’s a comment without a space after //, which gocritic flags:
- //owner := sdk.MustAccAddressFromBech32(deployment.ID.Owner)
+ // owner := sdk.MustAccAddressFromBech32(deployment.ID.Owner)📝 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.
| //owner := sdk.MustAccAddressFromBech32(deployment.ID.Owner) | |
| suite.PrepareMocks(func(ts *state.TestSuite) { | |
| bkeeper := ts.BankKeeper() | |
| bkeeper. | |
| On("SendCoinsFromAccountToModule", mock.Anything, mock.Anything, emodule.ModuleName, sdk.Coins{msg.Deposit.Amount}). | |
| Return(nil).Once() | |
| }) | |
| // owner := sdk.MustAccAddressFromBech32(deployment.ID.Owner) | |
| suite.PrepareMocks(func(ts *state.TestSuite) { | |
| bkeeper := ts.BankKeeper() | |
| bkeeper. | |
| On("SendCoinsFromAccountToModule", mock.Anything, mock.Anything, emodule.ModuleName, sdk.Coins{msg.Deposit.Amount}). | |
| Return(nil).Once() | |
| }) |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 443-443: commentFormatting: put a space between // and comment text
(gocritic)
🤖 Prompt for AI Agents
In x/deployment/handler/handler_test.go around lines 443 to 449, there's a
comment that begins with `//owner` which violates gocritic's requirement for a
space after the `//`; change that line to include a space (e.g., `// owner :=
sdk.MustAccAddressFromBech32(deployment.ID.Owner)`) or remove the comment if
unnecessary so the file passes gocritic formatting checks.
fixes issue when fractional of uakt remain in the balance after account is closed.
refactor escrow testsuite to ensure funds are tracked during account operations and match expected values
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...
!to the type prefix if API or client breaking changeCHANGELOG.md