SubAccount V2 (2/3): program + interface builders + program tests - #188
Merged
santy311 merged 17 commits intoAug 3, 2026
Merged
Conversation
Collaborator
Author
|
Stacked PRs — review/merge bottom → top:
|
santy311
changed the title
santy311
force-pushed
the
santhosh/subaccount-v2-program
branch
from
July 29, 2026 11:22
e67d530 to
d794d3c
Compare
tracy-codes
requested changes
Jul 29, 2026
tracy-codes
left a comment
tracy-codes
left a comment
Contributor
There was a problem hiding this comment.
Some feedback along with the specific comments:
- Can we please add test coverage for Secp256r1
SubACcountSignV2, k1/r1ToggleSubAccountV2, sol withdrawunder k1/r1? These are the cases from what I can tell are currently missing tests. - Per-subaccount state rent is currently unclaimable I think. close_swig instruction leaves orphaned V2 subaccounts. Or is it supposed to be handled exclusively by withdaw_from_sub_account ?
santy311
force-pushed
the
santhosh/subaccount-v2-program
branch
from
July 30, 2026 11:55
d794d3c to
5300b41
Compare
Collaborator
Author
added in 5300b41, new program/tests/sub_account_v2_secp_test.rs.
This is a complex case as we also have ATAs that will be present in the PDA and removing of actions inside the role with the newly added sub account actions, closing requires a separate PR of its own. |
tracy-codes
requested changes
Jul 31, 2026
santy311
added a commit
that referenced
this pull request
Aug 3, 2026
…dempotent Addresses the unresolved review threads on #188. is_swig_v2 (threads 1-4): the four V2 sub-account handlers guarded with `swig.wallet_bump == 0`, which reads only byte 40. On a V1 account that byte is the low byte of `reserved_lamports`, which a rent-carrying balance leaves non-zero, so the guard accepted nearly every real V1 account -- a mainnet census found 72 of 78 slipping through. Worst case is CreateSubAccountV2, which needs no wallet-address account: it would draw an id from bytes 44..48 and write the incremented counter back, silently adding 2^32 to a live V1 wallet's reserved_lamports, and create a working V2 sub-account under a V1 swig. Adds `require_swig_v2`, which length-checks before the unchecked read in `is_swig_v2` (the program owns accounts shorter than a Swig header) and is called before `split_parts_mut` takes the buffer mutably. All four handlers now share that one definition. Pre-granted scope (thread 5): creating an id the role already held `SubAccountV2All { id }` for failed with DuplicateV2SubAccountAction, because the auto-grant appended a second copy. Presence of the scope is the grant, so the append is skipped when it is already there; the end state is the same single action either way. Factors the action walk out of `authorize_scoped_v2` into `has_scoped_v2` rather than duplicating it. Tests: new sub_account_v2_version_guard_test.rs (7) builds a real V1 header from a mainnet reserve value and asserts each instruction fails with SignV2CannotBeUsedWithSwigV1 specifically -- all four fail against the old guard. Two more in sub_account_v2_test.rs cover the pre-grant repro and the unrelated-id case. 342 program tests pass.
This comment has been minimized.
This comment has been minimized.
tracy-codes
approved these changes
Aug 3, 2026
Groundwork shared by the V2 sub-account handlers, with no behavior change to existing instructions. - New `SwigError` variants for V2 failures (data-too-short, missing scoped permission, discriminator / owner / id / swig-id mismatch, disabled, invalid seed). - `append_actions_to_role`: extracts the tail-preserving realloc + SavedTail + add-actions sequence into a shared `pub(crate)` helper, so both UpdateAuthorityV1's AddActions path and CreateSubAccountV2's auto-grant use one hardened code path. - `perform_replace_all_operation` now rejects duplicate V2 scoped actions on the resulting role, covering both the ReplaceAll and AddActions mutation paths.
Adds the four V2 sub-account instructions (discriminants 18-21; the historical
gap at 8 is left unused) and their handlers. Authorization is default-deny: the
runtime handlers consult only scoped V2 actions — wallet-level All /
ManageAuthority / AllButManageAuthority never grant Sign/Withdraw/Toggle/Create.
- CreateSubAccountV2: draws a fresh id from the header counter, inits the
program-owned state PDA and the system-owned asset PDA, then auto-grants the
creator `SubAccountV2All { id }` via the shared append helper (grows the Swig
account, preserves the rent-claimer tail). Requires an explicit
`SubAccountV2Create` action — All alone is denied.
- SubAccountSignV2: CPIs as the asset PDA (invoke_signed), gated by
`SubAccountV2Sign` / `All`; keeps the V1 no-CPI stack-height guard. Swig is
writable because Secp authorities record their signature odometer in the swig
account during authentication.
- WithdrawFromSubAccountV2: moves SOL/SPL to swig_wallet_address only, gated by
`SubAccountV2Withdraw` / `All`.
- ToggleSubAccountV2: flips the state account's enabled kill-switch, gated by
`SubAccountV2Toggle` / `All`.
- All handlers require the wallet-address Swig generation (`wallet_bump != 0`).
Shared `authorize_scoped_v2` / `validate_v2_state` helpers. IDL regenerated.
- Five new `ClientAction` variants for the V2 permissions. - Instruction builders for CreateSubAccountV2, SubAccountSignV2, WithdrawFromSubAccountV2, and ToggleSubAccountV2, with Ed25519, Secp256k1, and Secp256r1 authority variants (account order + authority-payload construction mirroring their V1 counterparts; Secp256k1 uses `slot ++ counter ++ sig` with counter = odometer+1, Secp256r1 emits a precompile verify instruction).
litesvm program tests for the V2 sub-account handlers (they use the interface builders in this same PR). - sub_account_v2_test.rs (7): create / multi-create / SOL withdraw / toggle-disable / CPI sign / All-denied-create. - sub_account_v2_matrix_test.rs (13): the normative auth matrix — All / ManageAuthority / AllButManageAuthority denied for every runtime op; scope-limited grants (sign/withdraw/toggle-only); id-scoping of the All umbrella; cross-role sharing; self-grant recovery; atomic role-deletion; V1/V2 seed non-collision; duplicate-action rejection.
Add a reusable common::stability harness (SwigSnapshot capture + assert_others_stable) that snapshots a swig's roles, rent-claimer tail, and sub-account counter, then asserts unrelated roles and the claimer survive a size-modifying op byte-for-byte (matched by authority identity). - sub_account_v2_test: a create-V2 test proving a middle-role create does not corrupt other roles' authorities/permissions (refactored onto the harness). - swig_realloc_stability_test: a combination run over every account size-modifying instruction (add / update grow+shrink / remove authority, V2 create) with a rent claimer set throughout.
Replace the bare 108 index with TOKEN_ACCOUNT_STATE_OFFSET alongside the existing TOKEN_ACCOUNT_BASE_LEN, and name the AccountState::Initialized value it is compared against.
The V2 state discriminator is validated by SubAccountV2::check_discriminator in the state crate, so this variant was never constructed. Every V2 error is unreleased, so removing it shifts no live error code.
The Ed25519 builder passed the payer as read-only while the instruction declares it writable and both Secp builders pass it writable. The Secp variants cannot be changed to match: the signed account payload is built from the client's AccountMeta but rebuilt on-chain from the runtime AccountInfo, and a fee-paying account is always writable at runtime, so a read-only meta would desync the two and fail verification. Align Ed25519 with the declaration instead.
The V2 program tests were Ed25519-only, so the Secp authentication path for sub-account operations was never exercised on-chain. Add sign, SOL withdraw, and toggle under both Secp authority types. Sub-accounts are created by an Ed25519 role and the Secp authority is granted only the scoped action under test, keeping each case focused on one operation. Counters are read from the authority's odometer rather than assumed, so the tests do not depend on transaction ordering.
is_swig_v2 read the last 8 bytes of the header as a u64 and required the upper 7 to be zero. sub_account_counter now occupies bytes 44..48 of that window, so a migrated account started reading as V1 the moment its first V2 sub-account was created, silently and permanently. Narrow the window to wallet_bump ++ _padding, per tracy-codes' review suggestion: a non-zero bump with three zeroed reserved bytes. Pin the field offsets with const asserts so a field added after _padding cannot silently widen it again. This also fixes reserved_lamports == 0, which the old check read as V2. It is still a heuristic over bytes V1 used for a lamport balance: a V1 account misreads as V2 when reserved_lamports % 2^32 is in 1..=255. The sole caller, close_token_account_v1, falls back to the other authority, so a misread costs a comparison rather than correctness.
…dempotent Addresses the unresolved review threads on #188. is_swig_v2 (threads 1-4): the four V2 sub-account handlers guarded with `swig.wallet_bump == 0`, which reads only byte 40. On a V1 account that byte is the low byte of `reserved_lamports`, which a rent-carrying balance leaves non-zero, so the guard accepted nearly every real V1 account -- a mainnet census found 72 of 78 slipping through. Worst case is CreateSubAccountV2, which needs no wallet-address account: it would draw an id from bytes 44..48 and write the incremented counter back, silently adding 2^32 to a live V1 wallet's reserved_lamports, and create a working V2 sub-account under a V1 swig. Adds `require_swig_v2`, which length-checks before the unchecked read in `is_swig_v2` (the program owns accounts shorter than a Swig header) and is called before `split_parts_mut` takes the buffer mutably. All four handlers now share that one definition. Pre-granted scope (thread 5): creating an id the role already held `SubAccountV2All { id }` for failed with DuplicateV2SubAccountAction, because the auto-grant appended a second copy. Presence of the scope is the grant, so the append is skipped when it is already there; the end state is the same single action either way. Factors the action walk out of `authorize_scoped_v2` into `has_scoped_v2` rather than duplicating it. Tests: new sub_account_v2_version_guard_test.rs (7) builds a real V1 header from a mainnet reserve value and asserts each instruction fails with SignV2CannotBeUsedWithSwigV1 specifically -- all four fail against the old guard. Two more in sub_account_v2_test.rs cover the pre-grant repro and the unrelated-id case. 342 program tests pass.
Adds coverage for the ways each fix could silently regress. program/src/lib.rs (5 unit tests): - require_swig_v2 rejects a V1 header a bump-only check would accept - require_swig_v2 accepts a migrated account with a non-zero sub-account counter - require_swig_v2 rejects data shorter than the header at several lengths - is_swig_v2 pins the documented blind spot: reserved_lamports % 2^32 in 1..=255 reads as V2, 256 is the smallest value that reads as V1. Stated so narrowing or widening the heuristic is a deliberate choice. sub_account_v2_version_guard_test.rs (3 integration tests): - a header with the real wallet bump but dirty padding is rejected; pins the second half of the conjunction, which the old bump-only guard ignored - a truncated swig is rejected by the length guard rather than reaching the unchecked read at offset 40..44 - sub_account_counter must not flip a healthy V2 wallet to V1: creates four sub-accounts, then sign/withdraw/toggle must all still work. Guards the window fix from 0bf62a7 at the instruction level. sub_account_v2_test.rs (3 integration tests): - a pre-granted *specific* scope for the drawn id does not suppress the All auto-grant; the skip must key on SubAccountV2All exactly - another role holding All{id} does not suppress the acting role's auto-grant - after the skip, the pre-granted scope really authorizes sign/withdraw/toggle Each new test was checked against a mutated program: widening the is_swig_v2 window, dropping the length check, restoring the wallet_bump == 0 guard, and keying the skip on the wrong permission each fail the intended test and only that test. 437 / 111 / 233 across the three CI feature passes.
santy311
force-pushed
the
santhosh/subaccount-v2-program
branch
from
August 3, 2026 16:38
bade53e to
739e52d
Compare
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.
Summary
Program-crate implementation of SubAccount V2, plus the interface instruction builders and the program-level tests (the tests import the builders, so they live in the same PR).
program
Four new instructions (discriminants 18–21; the historical gap at 8 is left unused) and their handlers. Authorization is default-deny — the runtime handlers consult only scoped V2 actions; wallet-level
All/ManageAuthority/AllButManageAuthoritynever grant Sign/Withdraw/Toggle/Create.SubAccountV2All { id }via a shared, tail-preserving append helper. Requires an explicitSubAccountV2Createaction —Allalone is denied.invoke_signed), gated bySubAccountV2Sign/All; keeps the no-CPI stack-height guard. Swig is writable because Secp authorities record their odometer in the swig account during auth.swig_wallet_addressonly, gated bySubAccountV2Withdraw/All.SubAccountV2Toggle/All.wallet_bump != 0). IDL regenerated.interface
Five
ClientActionvariants + instruction builders for all four V2 instructions with Ed25519, Secp256k1, and Secp256r1 authority variants (mirrors the V1 builders).tests (litesvm)
sub_account_v2_test.rs(7): create / multi-create / SOL withdraw / toggle-disable / CPI sign / All-denied-create.sub_account_v2_matrix_test.rs(13): the normative auth matrix — overrides denied for every runtime op; sign/withdraw/toggle-only scopes; id-scoping of theAllumbrella; cross-role sharing; self-grant recovery; atomic role-deletion; V1/V2 seed non-collision; duplicate rejection.Test
Stack
main)🤖 Generated with Claude Code