fix: build P2ID asset from vault entry to match callback-flag vault keys#346
fix: build P2ID asset from vault entry to match callback-flag vault keys#3460xnullifier wants to merge 3 commits into
Conversation
Assets with AssetCallbackFlag::Enabled encode the flag in their vault key, so an asset built with `new FungibleAsset(faucet, amount)` does not match the vault entry even for the same faucet id. Resolve the asset from `account.vault().fungibleAssets()` via `FungibleAsset.fromVaultKey` instead, threading the account through `buildP2idTransactionRequest` callers. Also bumps @miden-sdk/miden-sdk to 0.15.8 and adds tests pinning the fromVaultKey usage and the vault-miss error path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughP2ID transaction construction now resolves fungible assets from the sender account’s vault and rebuilds them from vault keys. Multisig clients and browser examples pass the account through proposal-building flows, while Miden SDK dependencies move to version 0.15.8. ChangesP2ID vault asset flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Multisig
participant P2IDBuilder
participant AccountVault
participant FungibleAsset
Multisig->>P2IDBuilder: build request with account, faucet, and amount
P2IDBuilder->>AccountVault: find matching fungible asset
AccountVault-->>P2IDBuilder: return vault key
P2IDBuilder->>FungibleAsset: fromVaultKey(vault key, amount)
FungibleAsset-->>P2IDBuilder: return reconstructed asset
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/miden-multisig-client/src/transaction/p2id.ts (1)
73-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract vault-asset resolution into a named helper.
This adds lookup, validation, and conversion steps to the request builder, alongside prohibited
//implementation comments. AresolveFungibleAssetFromVault(account, faucet, amount)helper makes the behavior self-describing and keeps the builder focused on request construction.Proposed refactor
+function resolveFungibleAssetFromVault( + account: Account, + faucet: AccountId, + amount: bigint, +): FungibleAsset { + const faucetHex = faucet.toString(); + const assetFromVault = account + .vault() + .fungibleAssets() + .find(asset => asset.faucetId().toString() === faucetHex); + + if (!assetFromVault) { + throw new Error('Asset not found in vault, cannot do P2ID transaction'); + } + + return FungibleAsset.fromVaultKey(assetFromVault.vaultKey(), amount); +} + - // Build the asset from the vault entry rather than `new FungibleAsset(faucet, amount)`: - // assets with `AssetCallbackFlag::Enabled` encode the flag in their vault key, so a - // directly constructed asset would not match the vault entry even for the same faucet id. - const faucetHex = faucet.toString(); - const assetFromVault = account - .vault() - .fungibleAssets() - .find(asset => asset.faucetId().toString() === faucetHex); - if (!assetFromVault) throw new Error('Asset not found in vault, cannot do P2ID transaction'); - const asset = FungibleAsset.fromVaultKey(assetFromVault.vaultKey(), amount); + const asset = resolveFungibleAssetFromVault(account, faucet, amount);As per coding guidelines, “Prefer small, single-purpose functions over long multi-step procedures” and “Do not add inline comments (
// ...) in implementation code in multisig SDKs.”🤖 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 `@packages/miden-multisig-client/src/transaction/p2id.ts` around lines 73 - 82, Extract the vault lookup, missing-asset validation, and FungibleAsset conversion from the request builder into a named resolveFungibleAssetFromVault(account, faucet, amount) helper. Update the builder to call this helper, preserving the existing faucet matching and vault-key-based construction behavior, and remove the inline implementation comments.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/miden-multisig-client/src/transaction/p2id.ts`:
- Around line 73-82: Extract the vault lookup, missing-asset validation, and
FungibleAsset conversion from the request builder into a named
resolveFungibleAssetFromVault(account, faucet, amount) helper. Update the
builder to call this helper, preserving the existing faucet matching and
vault-key-based construction behavior, and remove the inline implementation
comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df9a9e35-0ca2-4ee9-8b94-5a0a311a0ae4
⛔ Files ignored due to path filters (4)
examples/_shared/multisig-browser/package-lock.jsonis excluded by!**/package-lock.jsonexamples/smoke-web/package-lock.jsonis excluded by!**/package-lock.jsonexamples/web/package-lock.jsonis excluded by!**/package-lock.jsonpackages/miden-multisig-client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
examples/_shared/multisig-browser/package.jsonexamples/_shared/multisig-browser/src/multisigApi.tsexamples/smoke-web/package.jsonexamples/web/package.jsonpackages/miden-multisig-client/package.jsonpackages/miden-multisig-client/src/multisig.tspackages/miden-multisig-client/src/transaction/p2id.test.tspackages/miden-multisig-client/src/transaction/p2id.ts
|
@0xnullifier The fix looks right, but the vault lookup and the execution read different state: the lookup uses Concrete regression window: fund a fresh multisig, finalize the consume-notes proposal, then call Could you pass the store's account at both That makes the lookup source identical to what |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
=======================================
Coverage ? 78.04%
=======================================
Files ? 167
Lines ? 31904
Branches ? 0
=======================================
Hits ? 24900
Misses ? 7004
Partials ? 0 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…pshot The vault lookup previously read this.account, a snapshot only refreshed by Multisig.syncState(), while executeForSummary runs against the web client's store, which other flows (e.g. consume-notes finalize) update directly. Funding a fresh multisig and finalizing before an explicit syncState() left the asset in the store but not the snapshot, throwing 'Asset not found in vault' where it previously succeeded. Both multisig.ts call sites (createP2idProposal and the co-signer rebuild path) now fetch the account via webClient.getAccount, falling back to the snapshot only if the store has no record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…example Centralizes the store-first account resolution (web client store with snapshot fallback) behind a public helper, and switches the example's P2ID call sites off the stale-prone multisig.account snapshot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@haseebrabbani addressed your comments |
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
The remaining difference between Rust and TS sdk is robustness. Missing faucet asset: Rust defaults to Disabled and continues (execution.rs); TS throws. Is that intended? Should they match?
4 participants
Summary
Closes #336.
Assets with
AssetCallbackFlag::Enabledencode the flag in their vault key, so an asset built withnew FungibleAsset(faucet, amount)does not match the vault entry even for the same faucet id.buildP2idTransactionRequestnow resolves the outgoing asset from the sender's vault (account.vault().fungibleAssets()) and rebuilds it viaFungibleAsset.fromVaultKey(vaultKey, amount), threading the sendingAccountthrough the builder and its callers (Multisig.createP2idProposal, the metadata rebuild path, and the browser example).@miden-sdk/miden-sdkto 0.15.8 for the newFungibleAsset.fromVaultKey/vaultKey()helpers (feat(web): expose fungible asset vault key helpers 0xMiden/web-sdk#247)fromVaultKeyusage and the vault-miss error pathWhy this does not break existing
MultisigflowsverifyProposalMetadataBindingrebuilds the request and immediately re-executes it viaexecuteForSummaryagainst the web client's store. Any state in which the new vault lookup could fail (asset absent) is state in which that execution already failed before this change — the lookup introduces no new failure mode, it only surfaces the same one earlier with a clearer message.tx_summarycommitment.FungibleAsset.fromVaultKey(vaultKey, amount)produces the same bytes as the oldnew FungibleAsset(faucet, amount)construction.this.accountis refreshed bysyncState(), which callers already must invoke after executed transactions (consuming a funding note, etc.) for other flows to work; this change adds no new sync requirement.MultisigAPI is unchanged. Only the exported helperbuildP2idTransactionRequestgained a parameter, and it should have not have any known consumersSummary by CodeRabbit
Bug Fixes
Tests
Chores