Deactivate the quarantine module and release all quarantined funds.#2737
Open
nagarajdivine wants to merge 13 commits into
Open
Deactivate the quarantine module and release all quarantined funds.#2737nagarajdivine wants to merge 13 commits into
nagarajdivine wants to merge 13 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves the quarantine module (wiring, sources, protos, CLI, docs, tests, simulation), updates exchange/marker/flatfees to stop using quarantine bypass, and adds an upgrade migration to refund quarantined records. ChangesQuarantine Module Removal
Sequence Diagram (migration flow): sequenceDiagram
participant Upgrader as upgrades.migrateQuarantineRecords
participant KV as QuarantineKV (prefixed iterator)
participant Bank as BankKeeper
participant ModuleAcc as ModuleAccount(quarantine.ModuleName)
Upgrader->>KV: iterate quarantine.RecordPrefix keys
KV-->>Upgrader: recordKey, recordValue
Upgrader->>Upgrader: parse recipient, unmarshal QuarantineRecord
Upgrader->>Bank: SendCoins(ModuleAcc, recipientAddr, Coins)
Bank-->>Upgrader: success / error (logged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
x/marker/keeper/send_restrictions_test.go (1)
1184-1396: ⚡ Quick winRemove the commented-out quarantine test block instead of leaving it in-place.
This is now dead test code for removed module behavior. Keeping ~200 lines commented makes the suite harder to maintain and review.
Proposed cleanup
-// func TestQuarantineOfRestrictedCoins(t *testing.T) { -// ... -// }If you want historical context, keep a short one-line comment near related tests (or rely on PR history) instead of the full commented implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/marker/keeper/send_restrictions_test.go` around lines 1184 - 1396, Remove the large commented-out test block for TestQuarantineOfRestrictedCoins: delete the entire commented function (the multi-line commented block beginning with "func TestQuarantineOfRestrictedCoins" and ending at its closing "}") and, if desired, replace it with a single-line comment noting historical context or rely on VCS history; ensure no other code references the test name TestQuarantineOfRestrictedCoins remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/app.go`:
- Line 680: Remove the no-op dead statement "unsanctionableAddrs =
append(unsanctionableAddrs)" — it appends nothing and should simply be deleted
from the function where unsanctionableAddrs is used; if the original intent was
to append the quarantine module address, replace the line with an explicit
append of that address (e.g., unsanctionableAddrs = append(unsanctionableAddrs,
quarantineModuleAddr)), otherwise remove the line entirely to eliminate the
redundant no-op.
In `@app/upgrades.go`:
- Line 556: The call to quarantine.ParseRecordKey ignores its error, so validate
its returned error and handle it before using toAddr (e.g., check the error and
return or log a clear error), ensuring you don't pass an invalid toAddr into
SendCoins; modify the code where quarantine.ParseRecordKey(fullKey) is called to
capture (toAddr, err), check err and abort/return an error upstream (or log and
skip) with context mentioning fullKey and the deployment/update function name so
SendCoins is only called with a valid toAddr.
- Around line 559-572: The migration currently aborts on a single SendCoins
error in migrateQuarantineRecords (app.BankKeeper.SendCoins call), whereas
unmarshal errors are logged and skipped; change this to log-and-continue
behavior: catch SendCoins errors inside the loop, use ctx.Logger().Error to
record the failure with key/fullKey, toAddr.String(), and error details, and
continue processing remaining records; also accumulate failures (e.g., append
error strings) and after the loop either log a summary of all failed refunds or
return a single aggregated error indicating how many refunds failed so the
upgrade can make maximum progress while still surfacing failures.
- Around line 540-579: Call migrateQuarantineRecords(ctx, app) from the upgrade
handler that removes the quarantine module/store (the handlers for "daisy-rc1",
"daisy", "edelweiss-rc1" or "edelweiss") and invoke it before the quarantine
store is deleted so records remain readable; locate the upgrade handler function
where quarantine is removed and insert a call to migrateQuarantineRecords(ctx,
app), handle and propagate any error it returns, then proceed with deleting the
quarantine module/store.
In `@x/exchange/keeper/keeper.go`:
- Around line 201-203: The comment above the assignment "ctx := ctxIn" in
keeper.go incorrectly states that the quarantine module is being bypassed;
update or remove that stale comment so it accurately reflects current behavior
(the incoming context is used as-is) and does not claim quarantine is bypassed.
Locate the comment near the assignment to the local variable ctx (symbols:
ctxIn, ctx) and either reword it to explain that the incoming context is
preserved/used or delete it entirely to avoid misleading future maintainers.
---
Nitpick comments:
In `@x/marker/keeper/send_restrictions_test.go`:
- Around line 1184-1396: Remove the large commented-out test block for
TestQuarantineOfRestrictedCoins: delete the entire commented function (the
multi-line commented block beginning with "func TestQuarantineOfRestrictedCoins"
and ending at its closing "}") and, if desired, replace it with a single-line
comment noting historical context or rely on VCS history; ensure no other code
references the test name TestQuarantineOfRestrictedCoins remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 688e45e4-9631-45c0-bf9d-280bbf449b15
📒 Files selected for processing (60)
app/app.goapp/upgrades.gointernal/provwasm/stargate_whitelist.goproto/cosmos/quarantine/v1beta1/events.protoproto/cosmos/quarantine/v1beta1/genesis.protoproto/cosmos/quarantine/v1beta1/query.protox/exchange/keeper/commitments_test.gox/exchange/keeper/fulfillment_test.gox/exchange/keeper/keeper.gox/exchange/keeper/keeper_test.gox/exchange/keeper/market.gox/exchange/keeper/market_test.gox/exchange/keeper/mocks_test.gox/exchange/keeper/msg_server_test.gox/exchange/keeper/payments.gox/exchange/keeper/payments_test.gox/flatfees/client/cli/cli_test.gox/marker/keeper/keeper_test.gox/marker/keeper/send_restrictions_test.gox/quarantine/client/cli/query.gox/quarantine/client/cli/tx.gox/quarantine/client/cli/util.gox/quarantine/client/cli/util_test.gox/quarantine/client/testutil/cli_test.gox/quarantine/client/testutil/common_test.gox/quarantine/client/testutil/query_test.gox/quarantine/client/testutil/tx_test.gox/quarantine/export_test.gox/quarantine/genesis.gox/quarantine/genesis_test.gox/quarantine/keeper/export_test.gox/quarantine/keeper/genesis.gox/quarantine/keeper/grpc_query.gox/quarantine/keeper/grpc_query_test.gox/quarantine/keeper/invariants.gox/quarantine/keeper/invariants_test.gox/quarantine/keeper/keeper_test.gox/quarantine/keeper/mocks_test.gox/quarantine/keeper/msg_server.gox/quarantine/keeper/msg_server_test.gox/quarantine/keeper/send_restriction_test.gox/quarantine/keys_test.gox/quarantine/module/module.gox/quarantine/msgs_test.gox/quarantine/quarantine_test.gox/quarantine/send_restriction_test.gox/quarantine/simulation/decoder.gox/quarantine/simulation/decoder_test.gox/quarantine/simulation/genesis.gox/quarantine/simulation/genesis_test.gox/quarantine/simulation/operations.gox/quarantine/simulation/operations_test.gox/quarantine/spec/01_concepts.mdx/quarantine/spec/02_state.mdx/quarantine/spec/03_messages.mdx/quarantine/spec/04_events.mdx/quarantine/spec/05_queries.mdx/quarantine/spec/06_client.mdx/quarantine/spec/README.mdx/quarantine/testutil/test_helpers.go
💤 Files with no reviewable changes (48)
- x/quarantine/spec/README.md
- proto/cosmos/quarantine/v1beta1/events.proto
- x/quarantine/simulation/genesis.go
- x/quarantine/send_restriction_test.go
- x/quarantine/spec/05_queries.md
- x/quarantine/export_test.go
- x/quarantine/keeper/invariants.go
- x/quarantine/spec/02_state.md
- x/quarantine/simulation/operations_test.go
- x/quarantine/simulation/decoder.go
- x/quarantine/genesis_test.go
- proto/cosmos/quarantine/v1beta1/query.proto
- x/quarantine/spec/01_concepts.md
- x/quarantine/client/testutil/cli_test.go
- x/quarantine/keeper/invariants_test.go
- x/quarantine/keeper/genesis.go
- x/quarantine/keeper/grpc_query_test.go
- x/quarantine/spec/04_events.md
- x/quarantine/spec/06_client.md
- x/quarantine/client/cli/util_test.go
- x/quarantine/module/module.go
- x/quarantine/simulation/decoder_test.go
- x/quarantine/client/testutil/tx_test.go
- x/quarantine/keeper/mocks_test.go
- x/quarantine/simulation/operations.go
- x/quarantine/client/testutil/query_test.go
- x/quarantine/keeper/msg_server.go
- x/quarantine/msgs_test.go
- x/quarantine/keeper/send_restriction_test.go
- x/quarantine/keys_test.go
- x/quarantine/keeper/grpc_query.go
- x/quarantine/genesis.go
- x/quarantine/client/cli/tx.go
- x/quarantine/simulation/genesis_test.go
- x/quarantine/testutil/test_helpers.go
- x/exchange/keeper/payments_test.go
- x/exchange/keeper/commitments_test.go
- internal/provwasm/stargate_whitelist.go
- x/quarantine/client/cli/util.go
- x/quarantine/keeper/msg_server_test.go
- x/marker/keeper/keeper_test.go
- x/quarantine/client/cli/query.go
- x/quarantine/keeper/export_test.go
- x/quarantine/spec/03_messages.md
- x/exchange/keeper/payments.go
- proto/cosmos/quarantine/v1beta1/genesis.proto
- x/exchange/keeper/msg_server_test.go
- x/quarantine/client/testutil/common_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 86-87: The workflow still tries to download a removed matrix
artifact (-04-coverage) causing the job to fail; update the artifact names
referenced by the download step(s) (e.g., the step named upload-coverage-report
and any other steps around line 177) to match the current matrix parts array
(["00","01","02","03"]) or change the download logic to iterate over the actual
matrix.parts instead of hard-coding "-04-coverage", so only existing artifacts
are downloaded.
- Line 56: Update the stale comment above the block that appends to
"pkgs.txt.part.03" to reflect the current sharding behavior: remove references
to quarantine and "part 03 and 04", state that only the "sanction" case is
special-cased now, and clarify that parts used are "00..03" (four parts). Locate
the comment immediately preceding the line that writes to pkgs.txt.part.03 and
replace its wording to match this current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6a9626f-e896-4f83-9504-a4952c7e006f
📒 Files selected for processing (1)
.github/workflows/test.yml
nagarajdivine
force-pushed
the
deprecated/quarantine-model
branch
from
77f5dc6 to
124a233
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
x/marker/keeper/send_restrictions_test.go (1)
1184-1396: ⚡ Quick winRemove the fully commented-out quarantine test block.
This is dead test code now and makes maintenance harder; delete it instead of keeping it commented.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/marker/keeper/send_restrictions_test.go` around lines 1184 - 1396, Remove the large fully commented-out test block for TestQuarantineOfRestrictedCoins in send_restrictions_test.go: delete the entire commented function (the /* ... */-style block starting with "// func TestQuarantineOfRestrictedCoins(t *testing.T) {" and ending at its closing comment) so no dead/test code remains; after removal, run go vet/go test to ensure no missing imports or references (check for symbols like TestQuarantineOfRestrictedCoins, newMarker, mustWithdraw, optIn, setAttr) and clean up any now-unused imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/upgrades.go`:
- Around line 540-549: The migration helper migrateQuarantineRecords will never
run because there is no upgrade entry wiring it and quarantine.StoreKey is
unregistered in the new binary; add a real upgrade in the upgrades map that
mounts the old quarantine.StoreKey for that upgrade, invokes
migrateQuarantineRecords in the upgrade handler (or in a pre-upgrade step) to
read and refund quarantined funds, and only after successful completion remove
the store via the upgrade store loader; specifically, add an upgrade name entry
to the upgrades map, ensure the upgrade handler calls
migrateQuarantineRecords(ctx, app), configure the StoreLoader to keep
quarantine.StoreKey mounted during that upgrade, and then remove/unmount the
store as part of the upgrade cleanup so quarantined records are returned before
the store is deleted.
- Around line 582-598: The migration currently logs SendCoins failures inside
migrateQuarantineRecords (where SendCoins, failCount, skipCount, successCount
are used) but still returns nil which would allow the quarantine store to be
removed and orphan funds; modify migrateQuarantineRecords to abort the upgrade
when any refund fails by returning a non-nil error if failCount > 0 (or
alternatively return a specific sentinel/upgrade error indicating partial
failure so the caller preserves the quarantine store), ensuring the caller of
migrateQuarantineRecords checks that error before deleting the quarantine store
or marking migration complete.
---
Nitpick comments:
In `@x/marker/keeper/send_restrictions_test.go`:
- Around line 1184-1396: Remove the large fully commented-out test block for
TestQuarantineOfRestrictedCoins in send_restrictions_test.go: delete the entire
commented function (the /* ... */-style block starting with "// func
TestQuarantineOfRestrictedCoins(t *testing.T) {" and ending at its closing
comment) so no dead/test code remains; after removal, run go vet/go test to
ensure no missing imports or references (check for symbols like
TestQuarantineOfRestrictedCoins, newMarker, mustWithdraw, optIn, setAttr) and
clean up any now-unused imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ed955cb-d4c3-4070-a8ea-acfa96a329a8
📒 Files selected for processing (62)
.changelog/unreleased/deprecated/2695-deprecate-quarantine-model.md.github/workflows/test.ymlapp/app.goapp/upgrades.gointernal/provwasm/stargate_whitelist.goproto/cosmos/quarantine/v1beta1/events.protoproto/cosmos/quarantine/v1beta1/genesis.protoproto/cosmos/quarantine/v1beta1/query.protox/exchange/keeper/commitments_test.gox/exchange/keeper/fulfillment_test.gox/exchange/keeper/keeper.gox/exchange/keeper/keeper_test.gox/exchange/keeper/market.gox/exchange/keeper/market_test.gox/exchange/keeper/mocks_test.gox/exchange/keeper/msg_server_test.gox/exchange/keeper/payments.gox/exchange/keeper/payments_test.gox/flatfees/client/cli/cli_test.gox/marker/keeper/keeper_test.gox/marker/keeper/send_restrictions_test.gox/quarantine/client/cli/query.gox/quarantine/client/cli/tx.gox/quarantine/client/cli/util.gox/quarantine/client/cli/util_test.gox/quarantine/client/testutil/cli_test.gox/quarantine/client/testutil/common_test.gox/quarantine/client/testutil/query_test.gox/quarantine/client/testutil/tx_test.gox/quarantine/export_test.gox/quarantine/genesis.gox/quarantine/genesis_test.gox/quarantine/keeper/export_test.gox/quarantine/keeper/genesis.gox/quarantine/keeper/grpc_query.gox/quarantine/keeper/grpc_query_test.gox/quarantine/keeper/invariants.gox/quarantine/keeper/invariants_test.gox/quarantine/keeper/keeper_test.gox/quarantine/keeper/mocks_test.gox/quarantine/keeper/msg_server.gox/quarantine/keeper/msg_server_test.gox/quarantine/keeper/send_restriction_test.gox/quarantine/keys_test.gox/quarantine/module/module.gox/quarantine/msgs_test.gox/quarantine/quarantine_test.gox/quarantine/send_restriction_test.gox/quarantine/simulation/decoder.gox/quarantine/simulation/decoder_test.gox/quarantine/simulation/genesis.gox/quarantine/simulation/genesis_test.gox/quarantine/simulation/operations.gox/quarantine/simulation/operations_test.gox/quarantine/spec/01_concepts.mdx/quarantine/spec/02_state.mdx/quarantine/spec/03_messages.mdx/quarantine/spec/04_events.mdx/quarantine/spec/05_queries.mdx/quarantine/spec/06_client.mdx/quarantine/spec/README.mdx/quarantine/testutil/test_helpers.go
💤 Files with no reviewable changes (48)
- x/quarantine/spec/04_events.md
- x/quarantine/spec/01_concepts.md
- x/quarantine/spec/03_messages.md
- x/quarantine/spec/README.md
- x/quarantine/keeper/send_restriction_test.go
- x/quarantine/spec/06_client.md
- x/quarantine/keeper/invariants_test.go
- x/quarantine/keeper/export_test.go
- x/quarantine/send_restriction_test.go
- x/quarantine/spec/02_state.md
- x/quarantine/export_test.go
- x/quarantine/simulation/decoder_test.go
- x/quarantine/keeper/genesis.go
- x/quarantine/simulation/operations_test.go
- x/quarantine/simulation/decoder.go
- x/quarantine/genesis_test.go
- proto/cosmos/quarantine/v1beta1/query.proto
- x/quarantine/spec/05_queries.md
- x/quarantine/keeper/grpc_query_test.go
- x/quarantine/simulation/operations.go
- x/marker/keeper/keeper_test.go
- x/exchange/keeper/payments.go
- x/quarantine/keeper/msg_server.go
- x/exchange/keeper/payments_test.go
- x/quarantine/client/testutil/query_test.go
- x/quarantine/module/module.go
- internal/provwasm/stargate_whitelist.go
- x/quarantine/client/cli/util_test.go
- x/quarantine/keeper/mocks_test.go
- x/quarantine/msgs_test.go
- x/quarantine/genesis.go
- x/quarantine/testutil/test_helpers.go
- x/quarantine/simulation/genesis.go
- x/quarantine/simulation/genesis_test.go
- x/quarantine/keeper/grpc_query.go
- x/exchange/keeper/commitments_test.go
- x/quarantine/client/testutil/cli_test.go
- x/quarantine/keeper/msg_server_test.go
- x/quarantine/client/cli/query.go
- x/quarantine/client/cli/tx.go
- proto/cosmos/quarantine/v1beta1/genesis.proto
- x/quarantine/client/testutil/tx_test.go
- x/quarantine/client/testutil/common_test.go
- x/quarantine/keeper/invariants.go
- x/quarantine/client/cli/util.go
- proto/cosmos/quarantine/v1beta1/events.proto
- x/quarantine/keys_test.go
- x/exchange/keeper/msg_server_test.go
nagarajdivine
force-pushed
the
deprecated/quarantine-model
branch
from
7ca3ad4 to
c0a4be0
Compare
SpicyLemon
reviewed
Jun 15, 2026
SpicyLemon
reviewed
Jun 15, 2026
SpicyLemon
reviewed
Jun 15, 2026
nagarajdivine
force-pushed
the
deprecated/quarantine-model
branch
from
957e5e6 to
5eb0d96
Compare
SpicyLemon
reviewed
Jun 15, 2026
SpicyLemon
reviewed
Jun 15, 2026
SpicyLemon
reviewed
Jun 15, 2026
SpicyLemon
changed the title
SpicyLemon
changed the title
SpicyLemon
reviewed
Jun 23, 2026
Comment on lines
+473
to
+475
| if failed > 0 { | ||
| return fmt.Errorf("ReleaseAllQuarantinedFunds: failed to release funds for %d record(s)", failed) | ||
| } |
Contributor
There was a problem hiding this comment.
Let's just log this error message and have the func always return nil.
If a migration returns an error, it stops the upgrade process and halts the chain. I'd rather that not happen. An error in here doesn't cause any problems that prevent any other parts of the chain from operating, so it's not worth the halt.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
closes: Remove the quarantine module #2695
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
Breaking Changes
Removals
Migrations
Behavior
Tests & Docs