fix(#451): UpdateBaseUnitMints requires the OLD vault to be empty - #462
Merged
Conversation
Ports upstream's `require_empty_vault_token_account` guard, which our port dropped. WHY THE EXISTING CHECK IS NOT ENOUGH The handler already refuses when `vault` / `c_tot` / `insurance` are non-zero. Those are the ENGINE's counters. They can all read zero while the SPL token account still holds a balance — tokens that arrived outside the accounted paths, or a residue the counters no longer track. Switching the mint then ORPHANS those atoms: the vault authority still owns them, but no instruction references that mint any more, so nothing can move them out. Not a theft, a one-way door. WHAT IS CHECKED, AND WHEN Only the mints that actually CHANGE, with the account indices conditional on that — mirroring upstream exactly. A caller switching only the secondary passes one extra account, not two; a no-op re-assert of the same mints passes none, so the common case stays cheap. Our `require_empty_vault_token_account` reads the amount rather than testing a return value, because our `verify_vault_token_account` returns `()` where upstream's returns the balance. Same obligation, and it keeps our STRICTER canonical-address pin (F-VAULT-FRAG), which upstream's variant does not carry. `EngineLockActive` matches upstream's error here and matches the header-counter check in the same handler, so a caller sees one code for "the market is not empty enough to do this". ABI CHANGE: additional accounts, conditionally. Rides the next deploy window alongside #412 — one coordinated SDK bump covers both. TEST HELPER: `configure_base_unit_mints` now derives the old-vault accounts from the market's CURRENT config and appends exactly what the handler will ask for. Eight callers pass different before/after mint combinations, and hand-computing which needs one account, two, or none is the sort of bookkeeping that goes quietly wrong — so the helper mirrors the handler's own branch structure. Every vault it builds is EMPTY, i.e. the passing case. VERIFIED new test `..._rejects_a_nonempty_old_vault` asserts BOTH halves: a one-atom vault blocks the switch AND leaves the config byte-identical, and the same call with an EMPTY vault succeeds and actually applies. The rejection alone would pass against a handler that rejected for any reason at all. NEGATIVE CONTROL with the amount check removed, that test FAILS at v16_wrapper.rs:21261; restored, it passes. full CI suite 603 passed / 21 failed — `scripts/ci-test.sh` reports "OK: failing set matches the allowlist exactly". Two tests surfaced the ABI change loudly rather than silently (`..._changes_primary_and_rotates`, `..._rejects_mismatched_decimals`), which is the append behaving as intended. UPSTREAM: this IS the upstream behaviour — ported, not invented. Upstream additionally takes an `expected_authority_epoch` and calls `require_authority_epoch_view`, which we do not; that is a separate divergence and is deliberately NOT bundled here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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 |
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
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.
Decision A — add the check at the next deploy and coordinate the SDK bump.
Ports upstream's
require_empty_vault_token_accountguard, which our port dropped.Why the existing check isn't enough
The handler already refuses when
vault/c_tot/insuranceare non-zero. Those are the engine's counters. They can all read zero while the SPL token account still holds a balance — tokens that arrived outside the accounted paths, or a residue the counters no longer track.Switching the mint then orphans those atoms: the vault authority still owns them, but no instruction references that mint any more, so nothing can move them out. Not a theft — a one-way door.
What's checked, and when
Only the mints that actually change, with account indices conditional on that, mirroring upstream exactly. Switching only the secondary passes one extra account, not two; a no-op re-assert of the same mints passes none, so the common case stays cheap.
Our
require_empty_vault_token_accountreads the amount rather than testing a return value, because ourverify_vault_token_accountreturns()where upstream's returns the balance. Same obligation — and it keeps our stricter canonical-address pin (F-VAULT-FRAG), which upstream's variant doesn't carry.EngineLockActivematches upstream's error here and the header-counter check in the same handler, so a caller sees one code for "the market is not empty enough to do this".ABI change: additional accounts, conditionally. Rides the next deploy window alongside #412 — one coordinated SDK bump covers both.
Test helper
configure_base_unit_mintsnow derives the old-vault accounts from the market's current config and appends exactly what the handler will ask for. Eight callers pass different before/after mint combinations, and hand-computing which needs one account, two, or none is the sort of bookkeeping that goes quietly wrong — so the helper mirrors the handler's own branch structure. Every vault it builds is empty, i.e. the passing case.Verified
v16_wrapper.rs:21261; restored → passesTwo tests surfaced the ABI change loudly rather than silently, which is the append behaving as intended.
Upstream
This is the upstream behaviour — ported, not invented. Upstream additionally takes an
expected_authority_epochand callsrequire_authority_epoch_view, which we don't; that's a separate divergence and deliberately not bundled here.🤖 Generated with Claude Code
https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D