feat: design system update, Para wallet integration, and multisig management improvements#5
Conversation
… and dev-only local key visibility - Show modal on login page if Miden Wallet extension is not installed, with link to Chrome Web Store - Add tooltip on Add Signers step showing steps to copy public key from Miden Wallet; auto-shows after 5s idle, hides after 3s - Hide LOCAL KEYS wallet source button and LOCAL SIGNER KEYS section in production (dev-only)
Body already sets font-geist, so children inherit it via CSS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults 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:
✨ 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 |
VAIBHAVJINDAL3012
left a comment
VAIBHAVJINDAL3012
left a comment
There was a problem hiding this comment.
Did a deeper pass over the new proposal flows (Signers / Send / Approve), the wallet hook, and the sync/execute handlers — mostly small, respectful suggestions inline.
The one I'd prioritize is the unsequenced Promise.all([syncProposals, syncState, getConsumableNotes]) at all the sync sites (flagged on MultisigContext); it's the likely cause of intermittent empty proposals/balances after execute or on a cold load.
One out-of-scope nit that isn't inline-able (pre-existing, untouched by this PR): copyToClipboard in src/lib/helpers.ts has no .catch, so a clipboard failure in an insecure/restricted context rejects unhandled.
Thanks for the work here — the design-system pass looks great.
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| await midenClient.sync(); | ||
| } | ||
| const [synced, state, notes] = await Promise.all([ |
There was a problem hiding this comment.
Small but important: this post-execute refresh — and the same pattern at the load/sync sites (~L420, L595, L700, L795) — runs syncState() in parallel with syncProposals() and getConsumableNotes(). Since syncState() is what hydrates the local account into the Miden store and the other two read from it, a lost race surfaces as No account header record found for given ID, so proposals and balances intermittently come back empty after an execute or on a cold load. Could we await syncState() first, then run the other two in parallel? Happy to share a sequenced version.
| setIncreaseThreshold(false); | ||
| }; | ||
|
|
||
| const handleRemoveSigner = async (commitment: string) => { |
There was a problem hiding this comment.
adjustedThreshold is derived only from the default threshold vs. the new signer count. If a per-procedure override (e.g. update_signers) sits above the post-removal signer count, the contract rejects the update at execution (per the OZ audit), so the proposal gets created and signed but reverts later. Also, when two removals are queued back-to-back each computes its threshold from the current state independently, so the combination can under/over-shoot. Might be worth validating against procedureThresholds (and any in-flight governance proposals) here.
|
|
||
| {/* Remove */} | ||
| <button | ||
| onClick={() => handleRemoveSigner(commitment)} |
There was a problem hiding this comment.
Remove fires the proposal immediately on click with no confirmation, and the button also renders for the current user's own commitment — so a single misclick (or removing yourself) creates an on-chain governance proposal. Could we add a confirm step, and maybe a subtle guard/warning when removing your own signer?
| const currentThreshold = detectedConfig?.threshold ?? 0; | ||
| const numSigners = detectedConfig?.numSigners ?? signerCommitments.length; | ||
| const myCommitment = multisig?.signerCommitment; | ||
| const displayThreshold = newThreshold ?? currentThreshold; |
There was a problem hiding this comment.
Two small robustness things around the threshold display: (1) newThreshold isn't reset after add/remove, so a lingering value plus a changed numSigners can render an out-of-range "3 of 2" until the next stepper click clamps it; (2) numSigners/currentThreshold fall back to 0 when detectedConfig is null, and the Add/Change sections below aren't gated by syncingState, so during a failed/pending sync they show "0 of 0" / "0 → 1" and stay submittable. Resetting newThreshold on config change and gating these sections on a loaded config would tighten it.
|
|
||
| const handleAddSigner = async () => { | ||
| setValidationError(null); | ||
| if (!newCommitment.trim()) { |
There was a problem hiding this comment.
handleAddSigner only checks non-empty here; format is validated later in normalizeCommitment, but a commitment that's already an existing signer isn't caught client-side. A quick signerCommitments.includes(normalized) guard would avoid a doomed proposal.
| </button> | ||
|
|
||
| <div className="flex-1 min-w-0"> | ||
| <div className="text-[13px] font-[500] text-[#111]"> |
There was a problem hiding this comment.
The security notice below says "Verify all transfer details before approving," but the row only shows proposal type + id + signature count — not the amount or recipient, which are exactly what a signer most needs to verify before signing. Could we surface amount/recipient from the proposal metadata here?
| catch { toast.error(`Failed to sign ${id.slice(0, 8)}…`); } | ||
| } | ||
| setSelectedIds([]); | ||
| toast.success("Signed selected proposals"); |
There was a problem hiding this comment.
This success toast fires unconditionally after the loop, even if every handleSignProposal in it threw (each failure already shows its own error toast). Tracking a success count and only toasting success when > 0 would avoid the conflicting "failed" + "signed" toasts.
| </div> | ||
|
|
||
| {/* Action button */} | ||
| {isReady ? ( |
There was a problem hiding this comment.
Two concurrency edges on these action buttons: (1) executingProposal/signingProposal are single ids, so only the acted-on row disables — while proposal A executes, B's Execute/Sign stay live and can interleave (each runs its own post-sync that clobbers the other's state, and on-chain you can hit nonce / pending-candidate conflicts). A global in-flight guard would help. (2) A signer who already signed still sees "Sign" until threshold is met, so they can re-submit their own signature — a small "already signed by me" check would prevent the redundant call.
| detectedConfig?.procedureThresholds | ||
| ); | ||
| const sigCount = proposal.signatures?.length ?? 0; | ||
| const isReady = sigCount >= propThreshold; |
There was a problem hiding this comment.
Minor/cosmetic: when detectedConfig is null, threshold falls back to 0 so getEffectiveThreshold returns 0 and isReady (sigCount >= 0) is always true — the row shows "Execute" even at 0 signatures. The execute handler does re-guard with status !== "ready" so it fails safely with a message, but the button is misleading in that state; gating the Execute affordance on propThreshold > 0 would match the handler.
|
|
||
| const handleConnect = (_address: string) => { | ||
| const pk = adapter.publicKey; | ||
| console.log(pk) |
There was a problem hiding this comment.
Looks like a leftover debug log — this prints the public key to the console on every connect. Safe to remove.
3 participants
Summary
Changes