Repository navigation
fix: double-submit guard, multisig hardening, contract-builder validation (#895, #897, #900, #906) - #1006
Open
omarima-10 wants to merge 3 commits into
Open
fix: double-submit guard, multisig hardening, contract-builder validation (#895, #897, #900, #906)#1006omarima-10 wants to merge 3 commits into
omarima-10 wants to merge 3 commits into
Conversation
…orokit#895) Rapid double-clicks (or a click racing a keyboard Enter submit) on the Send button could fire both submitTransaction/buildPreview calls before React commits the loading/disabled state, since that update only takes effect on the next render. This let a transaction submit twice, or the fee-estimation preview build run twice, from what the user experienced as a single click. Add a synchronous isSubmittingRef guard, checked and set before any async work starts in submitTransaction, buildPreview, and handleFormSubmit, and reset in each function's finally block. Unlike React state, the ref updates immediately and is unaffected by batching, closing the race window the disabled attribute alone cannot cover. Add regression tests for both the previewMode and direct-submit paths. The two clicks are dispatched as native events inside a single act() call rather than two separate fireEvent.click() calls, since the latter each flush React's render individually and land the second click on an already-disabled button regardless of the mutex, masking the bug.
… state (Sorokit#900) Two gaps in the multi-sig transaction builder's signer and persistence handling: 1. Nothing stopped two signers from sharing the same address. The threshold math would silently double-count that signer's weight, making the computed total weight and therefore the threshold validity wrong without any indication to the user. 2. Saved state in localStorage had no schema version. Any shape that didn't match the current validator exactly, including the previously-shipped state shape itself once this fix lands, was treated as corrupt and wiped rather than migrated, silently destroying a user's real saved transaction. Add findDuplicateSignerIndices, an inline per-signer error, a summary-level message, and a Next-button guard so duplicate addresses block progression past step 0. Add a schemaVersion field to the persisted BuilderState plus migrateBuilderState, which recognizes the pre-versioning shape (no schemaVersion field) and stamps it with the current version instead of discarding it, while still rejecting unrecognized future versions and genuinely malformed data. Add regression tests for both: duplicate detection (including whitespace-only differences and clearing the error once resolved) and migration (legacy load, re-save carrying the migrated data forward, current-version load, and future-version rejection).
… auth review (Sorokit#906) Three gaps in the Soroban contract interaction builder: 1. Complex argument types (vec, map, struct, tuple) only validated that the textarea content was syntactically valid JSON. A value that parsed fine but contained a null or empty-string element nested anywhere inside it, such as an empty slot in a tuple or a missing field in a struct, passed straight through and would only fail once it reached the contract, with a far less actionable error. 2. u128/i128 arguments were a bare text input with no numeric validation at all. A decimal, stray text, or an out-of-range value had nowhere to surface an error before being handed off as the argument value. 3. Nothing surfaced which address arguments a multi-auth method call would need separate authorization entries for, so there was no way to review that before invoking. Add findInvalidNestedValue, a recursive walk over the parsed JSON that flags a null/empty element at any depth with its exact path (e.g. value[1].backup), wired into the existing complex-type validation. Add parseSorobanInt128, which rejects anything that isn't a clean integer literal before it ever reaches BigInt(), and range-checks the result against the signed/unsigned 128-bit bounds, surfacing a plain inline error instead of letting BigInt throw. Add an Authorization Entries review panel: every address-typed argument on the selected method is listed with its current value and a "requires auth" toggle (defaulting to on), shown once a method takes more than one address argument. The reviewed entries are included in the onParamsReady payload. A real multi-auth XDR viewer that decodes actual SorobanAuthorizationEntry structures isn't feasible here. there is no auth-entry API anywhere in the client layer this component builds on (sorokit-core's client.ts has no such type), so this is scoped to the client-side review step that's actually buildable without inventing a new backend contract, disclosed as a partial fix in the PR. Add regression tests for all three: nested-value validation at multiple depths, BigInt parsing edge cases (decimals, non-numeric text, negative/out-of-range values, large valid values), and the auth-entry panel's visibility, defaults, toggling, and the data it hands back through onParamsReady.
|
@omarima-10 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! 🚀 |
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
1 participant
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
Four assigned issues in this batch, addressed as follows:
#895: prevent double-submit via synchronous mutex
closes #895
Changes
TransactionPanel.tsx: rapid double-clicks (or a click racing an Enter-key submit) on Send could firesubmitTransaction/buildPreviewtwice before React committed the loading/disabled state, since that update only takes effect on the next render. Added a synchronousisSubmittingRefguard, checked and set before any async work starts insubmitTransaction,buildPreview, andhandleFormSubmit, reset in each function'sfinally.Also found and fixed a real bug in the pre-existing two regression tests for this: both dispatched the two clicks via separate
fireEvent.click()calls, which each individually flush React's render viaact(), so the second click always landed on an already-disabledDOM button regardless of whether the mutex existed. The tests passed even with the mutex fully removed. Rewrote both to dispatch two native click events inside a singleact()call, which actually reproduces the race (verified: with the mutex removed, the tests now fail with "expected spy to be called 2 times, but got 3 times" and "...1 times, but got 2 times"; restored, they pass).Sub-issue 2 (UTF-8 byte memo limit) was already fully implemented in a prior commit (
d003590, closed as #686). Nothing to do.Sub-issue 3 ("Reset Form" clearing the source account): no such button/field exists in this file. Disclosed below.
Test plan: 59/59 existing + new tests pass in
TransactionPanel.test.tsx. Self-verified by reverting the mutex at all 4 sites and confirming both new tests fail with clear assertion messages, then restoring.#897: TransactionFeeCalculator.tsx, premise mismatch
No code change. Disclosed below.
#900: duplicate signer detection + schema migration for MultiSigTransactionBuilder
closes #900
Changes
findDuplicateSignerIndices: flags signers whose trimmed, non-empty address repeats an earlier signer's. Inline per-signer error, a summary message, and the "Next" button now blocks progression past step 0 while a duplicate exists.schemaVersionto the persistedBuilderStateandmigrateBuilderState, which recognizes the pre-versioning shape (noschemaVersionfield, what every save written before this change looks like) and stamps it with the current version instead of discarding it as corrupt, while still rejecting unrecognized future versions and genuinely malformed data.Test plan: 17/17 tests pass (9 original + 4 duplicate-detection + 4 migration). Self-verified each sub-fix independently: reverting the duplicate-address Next-button guard fails 3/4 new tests with "expected element not to be disabled"; reverting the legacy-migration branch fails 2/4 migration tests with a clear "expected default placeholder, got real migrated data" mismatch. Both restored and confirmed clean.
#906: ContractInteractionBuilder.tsx, recursive validation, BigInt parsing, auth review
closes #906
Changes
JSON.parse. AddedfindInvalidNestedValue, which recursively walks the parsed structure and flags anull/empty-string element at any depth with its exact path (e.g.value[1].backup), since Soroban has no implicit null inside these composite types.parseSorobanInt128, which rejects anything that isn't a clean integer literal before callingBigInt()(avoiding an uncaughtSyntaxError), and range-checks against the signed/unsigned 128-bit bounds, surfacing a plain inline error.address-typed argument on the selected method with its current value and a "requires auth" toggle (default on), shown once a method takes more than one address argument. Included in theonParamsReadypayload asauthEntries.Test plan: 41/41 tests pass (25 original + 16 new). Self-verified all three sub-fixes independently by reverting each in isolation and confirming the corresponding new tests fail with clear, specific messages, then restoring and confirming clean.
Disclosures
TransactionPanel.tsx. Confirmed by direct inspection and repo-wide grep. Not claimed as fixed.TransactionFeeCalculator.tsx): the manual fee input field and Low/Medium/High tier buttons described in the issue do not exist in this file. Confirmed by reading the full 275-line component: it is a read-only fee breakdown display (Stroops/XLM toggle plus refresh button only). Repo-wide grep found no such UI anywhere (SwapSimulator.tsxhas unrelated Low/Medium/High price-impact labels for swaps, not fee tiers; a separateFeeEstimator.tsxcomponent has its own already-implemented custom-fee input and speed multipliers, but that's a different file not named by the issue, and already has its own validation). No forced fix applied;closes #897intentionally omitted.SorobanAuthorizationEntryXDR structures would need an auth-entry API that doesn't exist anywhere in the client layer this component builds on (src/lib/client.tshas no such type or endpoint), building that from scratch would be a large, separate backend-and-frontend effort, not a minimal fix. The implemented panel reviews the real signer-candidate data this component already has (address arguments) and surfaces it for review before invocation, which is the practically useful part of the issue's intent given what's actually available to build on.