Atomic multi-operation batching with single passkey approval#330
Atomic multi-operation batching with single passkey approval#330Just-Bamford wants to merge 1 commit into
Conversation
|
@Just-Bamford is attempting to deploy a commit to the miracle656's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@Just-Bamford Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
This is really good work — the design is correct and the test coverage is excellent. The __check_auth change is the right approach: iterating _auth_contexts and relying on the fact that Soroban's single signature_payload commits to every context, so one WebAuthn assertion atomically authorizes the whole batch with the nonce consumed once. And batch_tests.rs covers exactly the cases I'd want — multi-context single signature, nonce-consumed-once atomicity, rejection of invalid context types, and replay protection. The BatchOperation/BatchResult SDK API is clean too.
One blocking issue before I can merge: sdk/src/useInvisibleWallet.ts has been fully reformatted (Prettier), so the diff is +1763/−1282 — but only ~1000 of those lines are real; the rest is whitespace/line-wrapping churn. Because this file contains the security-critical client signing path (signAuthEntry, low-S handling, auth-entry assembly), I can't safely confirm that nothing in the signing logic changed underneath a reformat that large. The repo also has no Prettier config, so this is your editor's default style rather than the project's.
Please revert the formatting-only hunks and keep just the batch additions (the new types + the batch() method), so the diff shows only the functional change. Once useInvisibleWallet.ts is a clean, minimal diff I'll re-review and merge — the feature itself is basically there. Thanks! 🎯
74cef8c to
939379b
Compare
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Miracle656
left a comment
There was a problem hiding this comment.
Thanks for keeping this current with main. The actual feature here is good — BatchOperation/BatchResult + batch(), the contract batch_tests.rs, and the lib.rs change. But the core issue from my last review is still here:
The PR still bundles a wholesale reformat of sdk/src/useInvisibleWallet.ts — +1751/-627, and even with diff -w ~1544 lines still differ, including unrelated existing declarations (e.g. register, deploy types) being rewritten. That makes the security-critical signing hook nearly impossible to review for the actual change, and it's exactly the kind of diff where a subtle regression in the auth/signing path would hide.
Please strip the reformat so the diff is only the batch feature: revert useInvisibleWallet.ts to main and re-apply just the additions (the BatchOperation/BatchResult types and the batch() method). A quick way: git checkout origin/main -- sdk/src/useInvisibleWallet.ts, then add back only your new batch code. The target is roughly +300/-5 on that file instead of +1751/-627.
Contract side (batch_tests.rs, lib.rs) and batch.test.ts are fine as-is. Once the hook diff is just the feature, happy to merge. 👍
544dec5 to
82c9272
Compare
82c9272 to
55654a6
Compare
Miracle656
left a comment
There was a problem hiding this comment.
Thanks for the iteration. Unfortunately this still can't merge as-is — and it's now a branch-base problem, not the earlier reformat one.
This PR's base is a stale pre-merge main (f76b58a), so its diff against current main deletes shipped SDK exports. sdk/src/index.ts here removes the barrels merged since you branched:
- export { …backup… } from './backup';
- export { …sep7… } from './sep7';
- export * from './crypto/prf';
- export * from './signMessage';
- export * from './bulkPayout';
- export * from './counterfactual';
- export * from './claimableBalance';
Those are live features (#334 backup, #337 sep7, #323 prf, #319 signMessage, #318 bulkPayout, #321 counterfactual, #325 claimable). Merging this would revert all of them.
To unblock: rebase onto current origin/main and force-push, so the diff contains only your batching changes:
git fetch origin
git rebase origin/main
# resolve conflicts in sdk/src/index.ts and sdk/src/useInvisibleWallet.ts by
# KEEPING all existing exports/hooks and ADDING your batch() additions
git push --force-with-leaseAfter the rebase the diff should be small and additive (the batch() method + BatchOperation/BatchResult types + the contract __check_auth change + your two test files). Also please don't include the package-lock.json churn (−5095) or the docs _meta.ts deletions — those are artifacts of the stale base. Ping me once rebased and I'll re-review.
Dismissed — posted from the wrong account by mistake; the maintainer review below supersedes it.
Description
Problem
Each action is a separate tx + passkey prompt. Composing several operations (approve + swap, or multi-send) into one signed transaction improves UX and atomicity.
this pr Closes #277
Solution
Implemented batch() API that collects multiple Soroban invocations into a single transaction, builds one auth payload, and gathers a single passkey assertion covering all auth contexts.
Changes
batch()method andBatchOperation/BatchResulttypes touseInvisibleWallet.ts__check_authinlib.rsto validate all auth contexts in batchbatch_tests.rs(atomicity, nonce consumption, context validation, replay protection)batch.test.tsAcceptance Criteria Met
✅ Multiple operations succeed/fail atomically (all-or-nothing)
✅ One passkey assertion authorizes all contexts
✅ Partial failure rolls back the entire batch
✅ Nonce advanced only once per batch
✅ Full test coverage
Files Modified
sdk/src/useInvisibleWallet.tssdk/src/__tests__/batch.test.ts(new)contracts/invisible_wallet/src/lib.rscontracts/invisible_wallet/src/batch_tests.rs(new)Usage