Remove Unbonding Logic - #18
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the legacy two-step unbond → redeem flow and updates the program + tooling so redemption is immediate (with optional closure of any legacy unbonding tickets for rent recovery).
Changes:
- Remove unbonding period configuration/update paths and deprecate the on-chain
unbonding_periodfield (kept for account layout compatibility, now set to0). - Remove the
unbondinstruction and updateredeemto take an explicitamountto burn/redeem immediately. - Update tests and operational scripts to match the new initialize/redeem APIs and legacy-ticket handling.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/vault-stake.test.ts |
Updates initialization + redemption tests to reflect immediate redeem and deprecated unbonding period. |
programs/vault-stake/src/processor.rs |
Removes unbond/update-config logic; changes redeem to immediate redeem(amount) with balance checks. |
programs/vault-stake/src/account_structs.rs |
Makes ticket optional on redeem to support legacy ticket closure. |
programs/vault-stake/src/lib.rs |
Updates public instruction signatures/docs (initialize + redeem). |
programs/vault-stake/src/state.rs |
Deprecates unbonding_period field; keeps legacy ticket type for compatibility. |
programs/vault-stake/src/events.rs |
Removes events tied to unbonding/update-config. |
programs/vault-stake/src/error.rs |
Removes error codes tied to unbonding flow; introduces/uses immediate redeem errors. |
scripts/vault-stake/initialize.ts |
Updates initialize CLI to remove unbonding period argument. |
scripts/vault-stake/redeem.ts |
Updates redeem CLI to require an explicit amount and optionally pass a legacy ticket PDA. |
scripts/vault-stake/query_all_unbonding_requests.ts |
Marks script deprecated; hardens IDL lookup and improves output for legacy ticket audit. |
scripts/deploy.sh |
Removes UNBONDING_PERIOD prompt/arg wiring from deployment flow. |
scripts/common.sh |
Removes display of UNBONDING_PERIOD. |
scripts/vault-stake/update_config.ts |
Deleted (unbonding period updates removed). |
scripts/vault-stake/unbond.ts |
Deleted (unbond instruction removed). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import yargs from "yargs"; | ||
| import {Program} from "@coral-xyz/anchor"; | ||
| import {VaultStake} from "../../target/types/vault_stake"; | ||
| import BN from "bn.js"; |
There was a problem hiding this comment.
BN is imported from bn.js just to construct the redeem amount, but the rest of the scripts use anchor.BN. Consider using new anchor.BN(...) and dropping the direct bn.js import for consistency and to avoid subtle parsing differences.
| import BN from "bn.js"; |
|
|
||
| const tx = await program.methods | ||
| .redeem() | ||
| .redeem(new BN(args.amount, 10, "le")) |
There was a problem hiding this comment.
new BN(args.amount, 10, "le") is misleading here: args.amount is already a JS number so base/endian are ignored, and the little-endian flag suggests a byte interpretation that isn't happening. Prefer a simpler new anchor.BN(args.amount) (or parse amount as a string if you want to avoid JS number precision issues).
| .redeem(new BN(args.amount, 10, "le")) | |
| .redeem(new BN(String(args.amount), 10)) |
| // DEPRECATED: unbonding period removed in v2. Kept for on-chain account layout compatibility. | ||
| pub unbonding_period: i64, |
There was a problem hiding this comment.
The deprecation comment says the unbonding period/tickets were removed in "v2", but other updated docs/scripts in this PR refer to removal in v0.0.5 (and "v1" legacy tickets). To avoid confusion for operators and auditors, align the versioning terminology across the codebase (pick one scheme and use it consistently).
| await new Promise(resolve => setTimeout(resolve, 15000)); | ||
| await program.methods.redeem() | ||
| // step 4 - attacker redeems immediately (no unbonding period) | ||
| await program.methods.redeem(new BN(user1Shares)) |
There was a problem hiding this comment.
user1Shares comes from getAccount(...).amount (a JS bigint), but new BN(user1Shares) will throw at runtime because bn.js 5.x doesn't accept bigint inputs. Convert to string (e.g. new BN(user1Shares.toString())) or construct the BN earlier from a string.
| await program.methods.redeem(new BN(user1Shares)) | |
| await program.methods.redeem(new BN(user1Shares.toString())) |
| // wait for >10 seconds unbonding period | ||
| await new Promise(resolve => setTimeout(resolve, 15000)); | ||
| await program.methods.redeem() | ||
| await program.methods.redeem(new BN(user2MintTokenBefore)) |
There was a problem hiding this comment.
user2MintTokenBefore is a bigint from getAccount(...).amount; new BN(user2MintTokenBefore) will throw because bn.js doesn't accept bigint. Use new BN(user2MintTokenBefore.toString()) (or reuse an existing BN) before calling redeem.
| await program.methods.redeem(new BN(user2MintTokenBefore)) | |
| await program.methods.redeem(new BN(user2MintTokenBefore.toString())) |
| it("redeems full balance in one call", async () => { | ||
| const mintBalance = (await getAccount(provider.connection, userMintTokenAccount)).amount; | ||
| if (mintBalance === BigInt(0)) { | ||
| return; // user already fully redeemed in a prior test; skip |
There was a problem hiding this comment.
This test conditionally returns early when mintBalance is 0, making it order-dependent and potentially skipping the assertion when tests are run in isolation or reordered. Prefer explicitly setting up the required state (e.g., deposit within the test) so the full-balance redeem behavior is exercised deterministically.
| return; // user already fully redeemed in a prior test; skip | |
| assert.fail("Test precondition violated: expected non-zero mint balance before redeeming full balance"); |
Removes the unbonding requirement for redemption.