Skip to content

sdk: append the payer Permission account on feed and user commands - #4228

Merged
martinsander00 merged 2 commits into
mainfrom
ms/infra-2343
Aug 25, 2026
Merged

sdk: append the payer Permission account on feed and user commands#4228
martinsander00 merged 2 commits into
mainfrom
ms/infra-2343

Conversation

@martinsander00

Copy link
Copy Markdown
Contributor

Summary of Changes

  • The Rust SDK appends the payer Permission account on doublezero feed create, feed delete, feed update, user delete, and user update when that account exists and the serviceability program owns it.
  • A signer that only holds feed-authority or user-admin on that account was denied with program error 0x8 (NotAllowed) because those commands never sent the extra account.
  • doublezero user create is unchanged. That instruction counts accounts to detect an optional tenant.

Part of https://github.com/malbeclabs/infra/issues/2343

Testing Verification

  • Feed create has a missing-account case and a present-account case. The present case expects the Permission account last on the instruction.
  • Feed delete, feed update, user delete, and user update stub a missing Permission account so the instruction matches the builder with no extra account.

@martinsander00
martinsander00 requested a review from a team August 21, 2026 23:52
@martinsander00
martinsander00 force-pushed the ms/infra-2343 branch 4 times, most recently from 6863d60 to 7604a8b Compare August 22, 2026 00:18
A signer that only holds those grants on its Permission account was denied on feed create/delete/update and user delete/update because those sends never included the trailing account.
@juan-malbeclabs

Copy link
Copy Markdown
Contributor

Review

I traced the on-chain side as well — authorize() / split_trailing_permission and the feed::{create,update,delete} / user::{delete,update} processors. The core mechanism is correct. For all five instructions the appended read-only Permission meta lands exactly where the program expects it:

  • feed create/update/delete: authorize() pulls it straight off accounts_iter after [feed, globalstate, payer, system].
  • user update: split_trailing_permission peels […, payer, system, permission] by PDA match; all four combinations of tenant-pair present/absent × permission present/absent parse correctly.
  • user delete: the positional parse ends at system, so the extra tail account is either consumed by authorize() (payer ≠ owner) or harmlessly ignored (self-delete).
  • The existence check correctly avoids the hard InvalidAccountData that authorize() raises for a non-existent Permission PDA, and a present-but-insufficient Permission account still falls back to the legacy allowlist, so no legacy key is locked out.

Four findings below. The first two are worth addressing before merge.


1. smartcontract/sdk/rs/src/commands/common.rs:9 — RPC failure is indistinguishable from "account absent"

if let Ok(account) = client.get_account(permission_pda) {

DZClient::get_account returns Err both when the account does not exist and when the RPC call fails (its retry predicate only covers Io / Reqwest / Middleware). A signer whose only authority is a FEED_AUTHORITY Permission runs doublezero feed create while the RPC node is lagging or returns a non-retryable error → the Permission account is silently dropped and the transaction fails with the exact 0x8 NotAllowed this PR exists to eliminate, with no hint that a lookup failed.

This is a re-introduction of the shape fixed in #4029 (sdk: don't cache RPC failures as absent permission account), which moved to get_account_with_commitment so that Ok(None) means definitive absence and Err means failure. The trait already exposes get_multiple_accounts(vec![pda]) -> eyre::Result<Vec<Option<Account>>> (client.rs:590), which gives the same distinction. Suggest the helper return eyre::Result<()> and propagate a genuine RPC error rather than degrade silently.

2. smartcontract/sdk/rs/src/commands/user/delete.rs:54 — the PR's own target path still fails

DeleteUserCommand::execute strips the user's multicast roles via UpdateMulticastGroupRolesCommand before sending the fixed DeleteUser:

for chunk in group_pks.chunks(MAX_GROUPS_PER_TRANSACTION) {
    UpdateMulticastGroupRolesCommand {}.execute(client)?;
}

That command (commands/multicastgroup/subscribe.rs:103) still sends no Permission account, while its processor (processors/multicastgroup/subscribe.rs:277-306) requires authorize(…, USER_ADMIN) whenever accesspass.user_payer != payer and the payer is not in the foundation allowlist.

So a signer holding only a USER_ADMIN Permission deleting someone else's user — precisely the scenario this PR targets — still fails with NotAllowed at the role-strip step, one instruction before reaching the fix, for any user carrying a publisher/subscriber role. The feed update|delete --force-unsubscribe paths route through the same command.

Either extend the helper to UpdateMulticastGroupRolesCommand or call out the limitation in the PR description / CHANGELOG. If extended, the variable-length extra_group_pks tail from #4120 is the one place where the trailing-account contract meets a variable account list, so it deserves a targeted test.

3. smartcontract/sdk/rs/src/commands/common.rs:6 — lookup is unmemoized

Every call pays a getAccountInfo round-trip. DeleteTenantCommand deletes users in a loop (commands/tenant/delete.rs), so an N-user tenant now issues N extra RPC calls for the same immutable-per-run PDA. The pre-#4060 DZClient::resolve_permission_account memoized this in permission_account_cache with explicit invalidation on CreatePermission / DeletePermission (#4002 / #4031). A per-client memo of only definitive results would restore that, and pairs naturally with finding 1.

4. crates/doublezero-serviceability-instruction/src/{feed.rs:9-13, common.rs:53-95} — docs now contradict the behavior

  • feed.rs states the change "intentionally diverges from today's Rust SDK feed commands, which use the plain (no-permission) execute_transaction; among authorize()-gated instructions the SDK feed commands are the odd ones out" — no longer true as of this PR.
  • build_with_permission's doc says the append is deferred and must be activated "here, in this one place… it then activates for every builder already assigned to this method at once", whereas this PR activates it caller-side for 5 of the ~40 gated instructions.

A future reader following either doc will draw the wrong conclusion about which commands carry the account. Both should record the caller-side partial activation and which commands it covers.


Minor nits, not blocking:

  • The new tests import AccountMeta from solana_sdk::message (a re-export of solana_instruction::AccountMeta, so it compiles) while common.rs uses solana_sdk::instruction.
  • commands/tenant/delete.rs:396 labels the new expectation 9. GetPermissionCommand: get(permission_pda), but it is a get_account call and is not part of the Sequence.

@juan-malbeclabs juan-malbeclabs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All four findings from my review are addressed in dfcef1a9.

  1. RPC failure vs. absenceappend_payer_permission_account returns eyre::Result<()> and goes through get_multiple_accounts(vec![pda])?, so Ok(None) is definitive absence and a real RPC error propagates instead of silently degrading to the 0x8 NotAllowed this PR exists to eliminate. All five call sites use ?.
  2. Role-strip pathUpdateMulticastGroupRolesCommand now appends the account, covering the strip that user delete / request-ban and feed update|delete --force-unsubscribe run first. split_trailing_permission matches the account by PDA against remaining[n - 3], so the variable-length extra_group_pks tail from #4120 parses correctly, and the new ..._extra_groups_with_permission_pda test exercises that batch + permission layout directly. No new deploy-ordering hazard: the trailing-permission tolerance on this instruction landed in #3966, already in released tags.
  3. MemoizationDZClient::permission_account_cache (Unresolved | Absent | Present) with invalidation on CreatePermission / DeletePermission. send_transaction_inner is the single send path (the legacy execute_transaction is gone), so no permission mutation through this client bypasses the hook; Update/Suspend/Resume change neither existence nor owner, which is all the append decision reads. Tenant delete over N users is one lookup instead of N.
  4. Docsfeed.rs and build_with_permission now record the caller-side partial activation and which commands it covers.

Two cosmetic leftovers, no need to block on them:

  • smartcontract/sdk/rs/src/commands/tenant/delete.rs:397 still labels the expectation 9. GetPermissionCommand: get(permission_pda); it is a get_multiple_accounts call now.
  • smartcontract/sdk/rs/src/client.rs:357 binds the cached Account as ref permission_pda — the name says PDA, the value is the account.

@martinsander00
martinsander00 enabled auto-merge (squash) August 25, 2026 01:09
@martinsander00
martinsander00 merged commit 10d12c1 into main Aug 25, 2026
38 checks passed
@martinsander00
martinsander00 deleted the ms/infra-2343 branch August 25, 2026 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants