Skip to content

fix: guard wallet send submission preflight#8

Open
tolga-tom-nook wants to merge 2 commits into
BitgesellOfficial:masterfrom
tolga-tom-nook:fix/send-preflight-guards
Open

fix: guard wallet send submission preflight#8
tolga-tom-nook wants to merge 2 commits into
BitgesellOfficial:masterfrom
tolga-tom-nook:fix/send-preflight-guards

Conversation

@tolga-tom-nook

@tolga-tom-nook tolga-tom-nook commented May 25, 2026

Copy link
Copy Markdown

Summary

  • revalidates the recipient address at submit time instead of relying only on the earlier change handler
  • recalculates spend/new-balance state immediately before submission
  • refuses to call sendrawtransaction when no serialized transaction has been generated
  • adds a focused Node VM regression harness for invalid-recipient, empty-transaction, and valid-send preflight behavior

Why

The send form can be submitted without the recipient field having passed through the change handler, or after stale form state leaves newTx empty. In that state the wallet could attempt a sendrawtransaction RPC with a null/stale transaction body instead of blocking locally and marking the recipient field invalid.

Hardening note

Follow-up commit 3dd1930 makes the review-assist merge bar visible in the repository test suite:

  • invalid recipient is marked invalid and never calls sendrawtransaction
  • empty serialized transaction never calls sendrawtransaction and does not disable the form
  • valid serialized transaction still sends exactly one RPC call, disables the form, preserves Basic auth, and includes the serialized transaction in the RPC body
  • npm test now lints nested src/**/*.js files and runs the send-form regression harness

Validation

  • node --check src/send/send.js
  • node --check test/send_preflight_test.js
  • npm test (eslint 'src/**/*.js' && node test/send_preflight_test.js)
  • git diff --check

Payout address if accepted: EVM/USDT-compatible 0x4a76c7E64C08cF29B59eFC640b4ada97A270d428.

@MyTH-zyxeon

Copy link
Copy Markdown

Maintainer-facing review assist for bitgesell-wallet #8 / Bitgesell #81. I am not claiming this bounty here; I reviewed the visible PR diff only, without installing dependencies, opening a wallet, using credentials, querying a live node/API, or performing any chain action.

This is a useful wallet safety slice: revalidating the recipient at submit time and refusing to call sendrawtransaction when newTx is empty reduces the risk of a stale form state reaching the RPC boundary.

A few checks I would keep visible before merge/acceptance:

  1. The !newTx guard now silently returns after calcAmountSpent(). If the serialized transaction is missing because amount/balance/fee state is invalid, the user may get no fresh visible error on submit. Please confirm calcAmountSpent() always marks the relevant field invalid, or add a small submit-time error state for the empty-transaction case.
  2. The recipient is revalidated with isAddressValid($sendToVal.value), which is good, but there is no regression test in this PR. A tiny DOM/fake helper test around send() would prove that a stale valid class, an invalid recipient, and an empty newTx all block before sendrawtransaction.
  3. Because this disables the fieldset only after the guards, please verify double-submit behavior when newTx is valid and the first fetch is still in flight. The patch does not appear to change that path, but it is relevant to wallet-send safety.
  4. It would help #81 acceptance hygiene to state whether npm test covers this send-form path or only broader smoke checks. If it does not cover the path, please paste the exact manual browser scenario maintainers should use.
  5. The PR body includes a payout address; before any bounty payment, maintainers should confirm whether #81 wants payout details in the PR body or in a separate approval/claim flow.

Suggested merge bar: submit-time invalid-recipient and empty-transaction branches are visibly covered, broader wallet tests pass, and maintainers confirm the payout/claim process for this focused wallet-send safety fix.

@tolga-tom-nook

Copy link
Copy Markdown
Author

Follow-up hardening pushed in 3dd1930: added a focused send-form preflight regression harness and wired it into npm test. It covers the review-assist merge-bar cases: invalid recipient blocks before sendrawtransaction, empty newTx blocks without disabling the form, and valid serialized transactions still send exactly one authenticated RPC call. Local validation: node --check src/send/send.js, node --check test/send_preflight_test.js, npm test, and git diff --check.

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