Skip to content

Multi-Transaction Signing for SignTransaction#551

Open
mraveux wants to merge 81 commits into
masterfrom
matheo/multi-tx-sign
Open

Multi-Transaction Signing for SignTransaction#551
mraveux wants to merge 81 commits into
masterfrom
matheo/multi-tx-sign

Conversation

@mraveux

@mraveux mraveux commented Mar 29, 2026

Copy link
Copy Markdown
Member

Summary

  • Extends the SignTransaction request to accept multiple transactions (TransactionData[] or Uint8Array[]) that are signed with a single password entry, enabling batch operations like combined staking + transfer flows
  • Adds a dedicated multi-transaction UI with a scrollable list of transaction cards (identicon, address, value, fee), summary totals, and a shared confirm button
  • Maintains full backward compatibility — single transactions (inline fields or single-item arrays) continue to work exactly as before, returning a single SignTransactionResult; only multi-item arrays return SignTransactionResults

Changes

Client types (client/src/PublicRequest.ts)

  • New TransactionData type (TransactionInfo without keyPath, since keyPath lives at request level)
  • SignTransactionRequestStandard now accepts either inline TransactionData fields or a { transactions: TransactionData[] | Uint8Array[] } object
  • New SignTransactionResults array type; ResultType and ResultByCommand updated accordingly

Request parsing (SignTransactionApi.js)

  • Auto-detects three input formats: inline single-tx fields, TransactionData[], or Uint8Array[] (serialized, for staking transactions)
  • Validates non-empty arrays, no mixed formats, standard-layout-only for multi-tx
  • Handles postMessage Uint8Array → plain object conversion
  • Removed the staking-type rejection — staking transactions are now supported via serialized format

Internal types (Keyguard.d.ts)

  • Parsed for Standard now uses transactions: Nimiq.Transaction[] instead of transaction: Nimiq.Transaction
  • Parsed for Checkout and Cashlink updated with Transform to also map transactiontransactions

UI handler (SignTransaction.js, SignTransaction.css, index.html)

  • Constructor branches between _renderMultiTransactionView and _renderSingleTransactionView based on transactions.length
  • Multi-tx view: transaction count header, scrollable card list with identicons, total value + total fees summary
  • Transaction count text is reactive to language changes via I18n.observer
  • Signing supports mixed staking/non-staking batches: uses transaction.sign(keyPair) when any staking tx is present, manual SignatureProof.singleSig otherwise
  • CSS scoped under #confirm-transaction.multi — no impact on SignMultisigTransaction which shares the same stylesheet

Translations (en.json)

  • 4 new keys: sign-tx-heading-multi, sign-tx-multi-count, sign-tx-multi-total-fees, passwordbox-confirm-txs
  • Non-English translations still need to be added

Other

  • PasswordBox.js: added passwordbox-confirm-txs button template
  • RequestParser.js: minor adjustment for shared parseTransaction() usage
  • demos/SignTransaction.html: comprehensive demo with format selector and multi-tx controls

Testing

Use the demo page at demos/SignTransaction.html. It provides a format selector (inline / TransactionData / Uint8Array) and controls to adjust the number of transactions. Test single-tx mode to verify backward compatibility, then switch to multi-tx to verify the batch UI, totals, and array result format.

mraveux added 5 commits March 29, 2026 16:25
Add TransactionData type and extend SignTransactionRequestStandard to
accept either inline single-tx fields or a transactions array. Return
type now includes SignTransactionResults for multi-tx responses.
Support signing multiple transactions in a single request, with both
TransactionData[] and serialized Uint8Array[] formats. Staking
transactions are signed using KeyPair.derive + transaction.sign()
instead of manual SignatureProof construction.

Adds a multi-transaction UI with per-transaction cards, totals, and
a "Confirm transactions" button variant.
Add translation keys for multi-tx heading, transaction count, total
fees label, and "Confirm transactions".
Rework the demo page to support inline, single-array, and multi-array
request formats with configurable transaction count and amounts.
- Remove unused `parsedRequest.plain` assignment in SignTransactionApi
- Remove staking-specific field parsing (validatorAddress, validatorImageUrl,
  amount) that was not on any type and unused by the handler
- Fix Parsed type for Checkout/Cashlink to map `transaction` → `transactions`
  matching what the parser actually produces
- Make transaction count text reactive to language changes
- Remove unused `.tx-label` CSS rule

@danimoh danimoh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a first part of the review, which includes everything but SignTransaction.js and the demo so far.
The included files have already been thoroughly reviewed.

The two missing files I have not thoroughly reviewed yet, but I am already aware of some things that require changing, so a second change request will follow for those files.

Comment thread client/src/PublicRequest.ts
Comment thread client/src/PublicRequest.ts Outdated
Comment thread client/src/PublicRequest.ts Outdated
Comment thread src/lib/RequestParser.js Outdated
Comment thread src/request/sign-transaction/index.html Outdated
// Deserialize using Nimiq's fromAny method
const tx = Nimiq.Transaction.fromAny(Nimiq.BufferUtils.toHex(txBytes));

// Validate transaction constraints (same as parseTransaction)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can consider calling tx.verify, and check that it fails because of an invalid signature, but not because of another error.

Comment thread src/request/sign-transaction/SignTransactionApi.js Outdated
Comment thread src/request/sign-transaction/SignTransactionApi.js Outdated
Comment thread src/request/sign-transaction/SignTransactionApi.js Outdated
Comment thread src/request/sign-transaction/SignTransactionApi.js Outdated

@danimoh danimoh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of SignTransaction.js.

I haven't looked at the demo yet, and also haven't tested.

Comment thread src/request/sign-transaction/SignTransaction.js Outdated
Comment thread src/request/sign-transaction/SignTransaction.js Outdated
Comment thread src/request/sign-transaction/SignTransaction.js Outdated
Comment thread src/request/sign-transaction/SignTransaction.js Outdated
Comment thread src/request/sign-transaction/SignTransaction.js Outdated
Comment thread src/request/sign-transaction/SignTransaction.js Outdated
Comment thread src/request/sign-transaction/SignTransaction.js Outdated
Comment thread src/request/sign-transaction/SignTransaction.js Outdated
Comment thread src/request/sign-transaction/SignTransaction.js Outdated
Comment thread src/request/sign-transaction/SignTransaction.js Outdated
mraveux added 23 commits April 19, 2026 20:13
Rename TransactionInfo (with keyPath) to TransactionInfoWithKeyPath and
TransactionData (without keyPath) to TransactionInfo, since senderLabel
makes "Data" a misleading name and the base type is more commonly used.

Addresses PR #551 review comment 1.
Use SignTransactionRequestCommon in the union to keep it truly common to
all SignTransactionRequests. Explicitly separate single-tx (with
recipientLabel) from multi-tx (without labels) branches.

Addresses PR #551 review comment 2.
Use SignTransactionResult[] directly instead of introducing a type alias,
as the alias adds indirection without meaningful abstraction.

Addresses PR #551 review comment 3.
In staking transactions, one side is always the staking contract and the
other the user's address, so they can never legitimately be equal.

Addresses PR #551 review comments 4 and 25.
The single-transaction view is the standard case and should come first
in the DOM for readability.

Addresses PR #551 review comment 5.
Replace the custom multi-only show/hide pattern with the existing hide-*
class convention used by the other headings. Remove now-unused multi-only
CSS rules.

Addresses PR #551 review comments 7 and 10.
For consistency with the single-transaction view where value and fee use
ids, switch transaction-count, transaction-list, total-value-amount and
total-fees-amount to ids. Also merge transaction-summary wrapper into
transaction-count directly.

Addresses PR #551 review comments 8 and 9.
Move the multi hide rule into the same selector block as the other
layout hide rules for consistency.

Addresses PR #551 review comment 11.
Use CSS mask-image to fade the top and bottom edges of the scrollable
transaction list, indicating there is more content to scroll. Also add a
comment explaining the negative-margin/padding pattern used to position
the scrollbar at the card edge.

Addresses PR #551 review comments 13 and 14.
These elements do not look like cards (no border-radius, no shadow), so
transaction-list-entry better describes their visual role.

Addresses PR #551 review comment 15.
Document why tx-details needs these properties: overflow:hidden clips the
text-overflow ellipsis, and min-width:0 lets the flex item shrink below
its intrinsic content width.

Addresses PR #551 review comment 16.
Instead of re-defining monospace font, size, ellipsis and overflow,
use the existing .address class from common.css. Only override the
opacity which differs from the default.

Addresses PR #551 review comment 17.
Add the 'total' class to the multi-tx total-value div so it picks up
the existing nim-symbol styling, removing the duplicate definition.

Addresses PR #551 review comment 18.
ConstructTransaction/ConstructMultisigTransaction now correctly accept
the base TransactionInfo type (without keyPath) after the rename, making
them more permissive. The Parsed type for SignTransactionRequestStandard
now omits the raw transaction fields (sender, recipient, value, etc.)
that are consumed into Nimiq.Transaction[] during parsing.

Addresses PR #551 review comments 19 and 20.
Parse each transaction entry individually instead of making assumptions
about all entries based on the first one. This allows mixing
TransactionInfo objects and Uint8Array in the same array. Also simplify
Uint8Array validation, remove unnecessary comments, and fix senderLabel
to only be extracted from single-tx TransactionInfo entries (it does not
exist on the request for multi-tx).

Addresses PR #551 review comments 22, 23, 24, 27, 28, 29, and 30.
It is only used within the constructor, so a local variable suffices.

Addresses PR #551 review comment 31.
The method names _renderSingleTransactionView and
_renderMultiTransactionView are descriptive enough without a prose
comment. Keep only the @param annotation.

Addresses PR #551 review comments 32 and 33.
Control single/multi transaction view visibility via the .multi class
on the card element, using CSS rules instead of JS display-none
toggling. This is more elegant and consistent with the hide-* pattern.

Addresses PR #551 review comment 34.
Replace manual translatePhrase + replace + language observer with the
built-in translateToHtmlContent helper, which handles variable
substitution and language change updates automatically.

Addresses PR #551 review comment 35.
When aggregating multiple transactions, totalValue and totalFee can
potentially exceed Number.MAX_SAFE_INTEGER, causing loss of precision
when converting to Number for display. Add a check during parsing to
reject such requests early.

Addresses PR #551 review comment 36.
Display both sender and recipient addresses with identicons and an
arrow between them so the user can see where funds come from. Make
entries clickable to open the details overlay with the full recipient
address. Show transaction data (contract creation, staking info, etc.)
using TransactionDataFormatting to prevent signing unwanted operations
in what looks like a harmless 0 NIM transaction.

Addresses PR #551 review comments 37, 38, and 39.
Instead of a blanket staking/non-staking split, decide per transaction
whether to use transaction.sign() (auto staker/validator proof) or
manual signing (HTLC, basic, or user-provided staking proof). Staking
transactions with user-provided signature proofs in the recipient data
now correctly defer to manual signing. Improve comments explaining both
signing paths.

Addresses PR #551 review comments 40, 41, and 42.
mraveux added 14 commits May 18, 2026 11:04
"Locked" framed the new delegation as a restriction; "staked" reflects
the user's actual intent and is consistent with the rest of the staking
copy.
The switch-validator view had no fee display; if either of the two
transactions carried a non-zero fee, the user could not see it before
signing. Mirror the unstaking-view pattern: sum the fees across both
transactions and reveal a small "+ X NIM fee" row beneath the validator
cards when the total is > 0.
Aligns the unstaking view with every other view in the file, which
already identifies its value and fee elements by id. Drops the
redundant -value suffix on the span names to match the single-tx
(#value, #fee) and switch-validator (#switch-validator-fee) naming.
The descriptive .unstake-duration-note scaffolding stays a class.
Previously, a request with transactions set to a non-array (object,
string, etc.) silently fell through to the single-transaction path,
yielding a misleading error from parseTransaction or worse, accepting
the request if legacy fields happened to be present. Validate the
shape up front: presence of transactions implies multi-tx mode and
requires an actual array.

Also moved the empty-array check above the layout-restriction check so
a malformed empty array reports the more specific error.
Transaction.fromAny(BufferUtils.toHex(entry)) was a four-way dispatch
through a string round-trip when we already know the input is bytes.
Transaction.deserialize(entry) is the direct API for that shape and
makes the intent explicit. Wrap in try/catch and re-throw as
InvalidRequestError so a malformed payload surfaces with the right
error category and message instead of bubbling up as Unclassified.
parseTransaction already rejects non-objects, missing senders, missing
recipients, and other shape errors with specific InvalidRequestError
messages. The local guard and trailing throw duplicated that work and
replaced specific failure messages with a generic "Invalid transaction
entry" wrapper.
A single transaction whose value or fee already exceeds
Number.MAX_SAFE_INTEGER would also lose precision in the Number(...)
conversions used by the renderers. Skipping the check for single-tx
arrays let that case slip through. Always run the check; the extra
iteration in the common single-tx case is negligible.
parseLabel already returns undefined for an undefined input (allowEmpty
defaults to true), so the guard was a no-op — calling parseLabel
unconditionally produces the same parsedRequest.senderLabel value.
senderLabel was being parsed in three places: from the first array
entry (typed-absent after lifting senderLabel to request level), from
the legacy single-tx else block, and again in switch-validator and
unstaking. Drop the first two — they conceptually belong with each
layout's parsing — and add explicit parses in the standard, checkout
and cashlink branches where the field is actually used.

The cashlink branch also reshapes from "fires only when cashlinkMessage
is set" to "fires for all cashlink requests, with the message check
moved inside". The senderLabel parse for cashlink-without-message
previously fell through to the legacy else block; with that gone, the
branch must run unconditionally.

The standard-branch senderLabel parse is guarded by 'senderLabel' in
request because the multi-tx variant of SignTransactionRequestStandard
doesn't carry the field — the in-operator narrows the union to the
variants that do.

Net behavior: same parsedRequest.senderLabel for every existing path.
Switch-validator and unstaking stop being parsed twice.
senderLabel and recipientLabel are only meaningful for the single-tx
variants of the standard request — the multi-tx variant omits both
fields in the type. Gate both parses on transactions.length === 1 so
a multi-tx caller can't attach an extraneous label that the parser
would silently accept.
Restructures the switch-validator branch to address several review
threads at once:

- Destructure the two transactions and their parsed staking data with
  descriptive names instead of indexing parsedRequest.transactions
  repeatedly.
- Replace the optional-chaining check with explicit && form (the
  eslint version in this repo doesn't parse ?.).
- Rename the sender-equality check from "same staker" to "same
  fee-paying sender" and document why: the sender is the fee-payer,
  not the staker, but staker equality still holds via the proof-check
  rejection that forces both staking proofs to be signed by the
  request's keyPath.
- Add three sanity checks on how the two transactions must relate for
  a switch to make sense: full deactivation (newActiveBalance === 0),
  full reactivation under the new delegation (reactivateAllStake), and
  set-active-stake must not be valid after update-staker.
- Derive validatorAddress from update-staker's newDelegation rather
  than parsing it from the request. The signed delegation is
  authoritative; an externally-supplied validatorAddress is either
  redundant or wrong. Drops the now-obsolete cross-check.
- Update the Parsed<...SwitchValidator> Transform in Keyguard.d.ts to
  match the request-type change (validatorAddress no longer needs to
  be Omit'd because it isn't a request field anymore).

Also lifts _hasStakerOrValidatorProof out of the Uint8Array-only
branch in the shared entry-parsing loop so it applies uniformly to
TransactionInfo entries too. Without this, a caller could embed a
forged staker proof in recipientData and bypass the rejection — which
would undermine the staker-equality argument above for switch-validator
(and for unstaking, addressed by the next commit).
Renames the unstaking branch's transaction and type locals from opaque
tx0/tx1/tx2 + t0/t1/t2 to setActiveStakeTx/retireStakeTx/removeStakeTx
and setActiveStakeType/retireStakeType/removeStakeType so each role is
visible at the call site.

Reworks the sender-equality check to match the switch-validator branch:
- Error reworded from "must be bound to the same staker" to "must share
  the same fee-paying sender and payout address" — the check verifies
  fee-payers and payout-address equality, not staker equality. Staker
  equality holds because _hasStakerOrValidatorProof (lifted to apply to
  all entries in the previous commit) rejects user-provided proofs and
  forces all three proofs to use the request's keyPath.
- Comment shortened and cross-references the switch-validator branch
  for the longer staker/sender explanation.

parseAddress for validatorAddress reformatted to one-arg-per-line to
match the codebase convention.
The comment referenced tx0/tx1 (the pre-rewrite locals in the parser)
and called the binding target the "staker", which the recent rewrites
established as a misnomer. Updated to use the new parser names
(setActiveStakeTx, removeStakeTx) and accurate "fee-paying sender"
terminology.
The new switch-validator and unstaking code introduced five eslint
errors that didn't exist on master:

- Replace 12 BigInt literals (0n) with BigInt(0). The eslint 5.x parser
  in this repo can't parse the literal syntax; the function call form
  is semantically identical and accepted.
- Drop a stray blank line at the end of the transactions-array branch
  in SignTransactionApi.js (padded-blocks).
- Drop the unused 'request' parameter from _setupTxListOverlay; the
  method already uses this._request when building the list later.
- Inline-paren the JSDoc casts that previously wrapped the cast
  expression across a line break, so the '=' isn't left dangling
  (operator-linebreak).
@mraveux
mraveux force-pushed the matheo/multi-tx-sign branch from 92c9b3f to e2467d4 Compare May 18, 2026 09:07

@danimoh danimoh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at e2467d460d136d107bf0d62973df22a6a4e049f8.

Comment on lines 318 to 321

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as tx-total-value, so can be removed.

this.$el.classList.add('tx-list-details-open');
$infoIcon.setAttribute('aria-expanded', 'true');
this.$txListDetails.setAttribute('aria-hidden', 'false');
$closeTxList.focus();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such focus() behavior is mising for the address overlay (focus close button of address overlay on open and re-focus close button of transaction list on close).
It should be consistent. Either do it for both or leave it our for both.
I think it's fine to remove the focus behavior, as we're not doing it for other flows either.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah i'll remove the focus as it's redundant with the esc keybind 👍

Comment on lines +477 to +485
window.addEventListener('keydown', event => {
if (event.key !== 'Escape') return;
// Close in reverse layering order: address-details sits above tx-list when both are open.
if (this.$el.classList.contains('account-details-open')) {
this._closeDetails();
} else if (this.$el.classList.contains('tx-list-details-open')) {
this._closeTxList($infoIcon);
}
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only adds the event listener for requests with a transaction list overlay. Requests without a transaction list overlay do not currently get the event listener for closing the address overlay.

Move the event listener registration to the constructor instead, to enable it for all requests.

Comment on lines +27 to +31
/* Back button is shown only on checkout and cashlink. */
#confirm-transaction.standard .page-header-back-button,
#confirm-transaction.multi .page-header-back-button,
#confirm-transaction.switch-validator .page-header-back-button,
#confirm-transaction.unstaking .page-header-back-button { display: none; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small cleanup: newlines as we usually do them and reducing duplication by using :not().

Suggested change
/* Back button is shown only on checkout and cashlink. */
#confirm-transaction.standard .page-header-back-button,
#confirm-transaction.multi .page-header-back-button,
#confirm-transaction.switch-validator .page-header-back-button,
#confirm-transaction.unstaking .page-header-back-button { display: none; }
/* Back button is shown only on checkout and cashlink. */
#confirm-transaction:not(.checkout, .cashlink) .page-header-back-button {
display: none;
}

Comment on lines +18 to +25
/* Headings: hide all, opt the active layout's heading back in. */
#confirm-transaction .page-header > h1 { display: none; }
#confirm-transaction.standard .heading-standard,
#confirm-transaction.checkout .heading-checkout,
#confirm-transaction.cashlink .heading-cashlink,
#confirm-transaction.multi .heading-multi,
#confirm-transaction.switch-validator .heading-switch-validator,
#confirm-transaction.unstaking .heading-unstaking { display: block; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small cleanup: newlines as we usually do them.

Suggested change
/* Headings: hide all, opt the active layout's heading back in. */
#confirm-transaction .page-header > h1 { display: none; }
#confirm-transaction.standard .heading-standard,
#confirm-transaction.checkout .heading-checkout,
#confirm-transaction.cashlink .heading-cashlink,
#confirm-transaction.multi .heading-multi,
#confirm-transaction.switch-validator .heading-switch-validator,
#confirm-transaction.unstaking .heading-unstaking { display: block; }
/* Headings: hide all, opt the active layout's heading back in. */
#confirm-transaction .page-header > h1 {
display: none;
}
#confirm-transaction.standard .heading-standard,
#confirm-transaction.checkout .heading-checkout,
#confirm-transaction.cashlink .heading-cashlink,
#confirm-transaction.multi .heading-multi,
#confirm-transaction.switch-validator .heading-switch-validator,
#confirm-transaction.unstaking .heading-unstaking {
display: block;
}

Comment thread types/Keyguard.d.ts Outdated
Comment on lines 351 to 352
'transactions' | 'validatorImageUrl'
| 'fromValidatorAddress' | 'fromValidatorImageUrl',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fit in one line.

Comment on lines +160 to +162
// Same fee-payer, not same staker — staker is identified by the staking proof, which
// can differ from the tx sender. Staker equality holds because _hasStakerOrValidatorProof
// rejects user-provided proofs, forcing both proofs to use the request's keyPath.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I clarified the comment a bit more.
Note that this check's / comment's correctness also depends on enforcing the sender type to be basic, as commented in my last review.

Suggested change
// Same fee-payer, not same staker — staker is identified by the staking proof, which
// can differ from the tx sender. Staker equality holds because _hasStakerOrValidatorProof
// rejects user-provided proofs, forcing both proofs to use the request's keyPath.
// Check that the sender / fee payer is the same for both transactions. Note that this is not necessarily the same
// as the staker because the staker is identified by the staking proof which can differ from the transaction sender.
// However, that both transactions are for the same staker is effectively enforced by us currently requiring
// !_hasStakerOrValidatorProof, which rejects user-provided staking proofs, resulting in both staking proofs
// being generated for the request's keyPath. By this, the same staker will be used for both transactions,
// and it will match the transaction senders for that keyPath as we're enforcing the senders to be of basic type.
// If we'd allow user-provided staking proofs in the future, we'd need to add a check that the transaction stakers
// match and are the same as the transaction senders for the simplified switch-validator flow.

Comment on lines +231 to +233
// Fee-payer for the first two, payout address for the third — not a staker-equality
// check (see switch-validator). Binding removeStakeTx.recipient to the staker prevents
// redirecting unbonded NIM to an attacker via benign-looking labels.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is contradicting itself a bit. It says that we're not checking the staker here, but then goes on to say that it binds the recipient to the staker.

Note that this check's / comment's correctness also depends on enforcing the sender type to be basic, as commented in my last review.

Suggested change
// Fee-payer for the first two, payout address for the third — not a staker-equality
// check (see switch-validator). Binding removeStakeTx.recipient to the staker prevents
// redirecting unbonded NIM to an attacker via benign-looking labels.
// Check that the fee payer is the same for all three transactions. For the incoming staking transactions setActiveStakeTx
// and retireStakeTx the fee is paid by the transaction sender and for the outgoing staking transaction removeStakeTx
// the fee is paid by the staker from the removed stake. We check the setActiveStakeTx and retireStakeTx senders
// to be the same, and if they are basic senders as checked separately, they are also the same as removeStakeTx's
// staker as they're signed by the same keyPath.
// Note that the staker of the setActiveStakeTx and retireStakeTx is determined by the staking proofs of those
// transactions, which might be different from their transaction senders. However, that those stakers also match
// the setActiveStakeTx and retireStakeTx senders and the removeStakeTx staker is effectively enforced by us
// currently requiring !_hasStakerOrValidatorProof, which rejects user-provided staking proofs, resulting in both
// staking proofs being generated for the request's keyPath. By this, the same staker will be used for both
// transactions, and it will match the setActiveStakeTx and retireStakeTx senders at keyPath for the basic sender
// type and the removeStakeTx staker for keyPath.
// If we'd allow user-provided staking proofs in the future, we'd need to add a check that the transaction stakers
// match and are the same as the transaction senders for the simplified unstaking flow.
//
// We also enforce the recipient of the removeStakeTx that is receiving the unstaked funds to match that same
// user and staker address. This prevents sending unstaked NIM to an attacker via benign-looking labels.

|| !removeStakeTx.recipient.equals(setActiveStakeTx.sender)) {
throw new Errors.InvalidRequestError(
'unstaking transactions must be bound to the same staker',
'unstaking transactions must share the same fee-paying sender and payout address',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'unstaking transactions must share the same fee-paying sender and payout address',
'unstaking transactions must share the same fee-paying sender, staker and payout address',

mraveux added 14 commits June 7, 2026 16:15
Collapse the let+reassignment chain into a single ternary so the
'multi' CSS class no longer needs a string-widening annotation.
The switch-validator/unstaking branches were no-ops (viewClass
already equalled request.layout there).
The method builds a single sender/recipient cell, not a whole row, so
rename _createTransactionAddressRow -> _createTransactionAddressCell and
the .tx-row class / $row local to .tx-address-cell / $cell. Move the
AddressInfo construction into the method since it depends only on the
address already passed in, dropping the second parameter and the two
up-front constructors in _createTransactionListEntry.
The transaction list shows entries, not cards: rename the $card local
in _renderMultiTransactionView to $entry and update the "compact cards"
comment on .transaction-list's max-height accordingly.
The icon-bundling build step grep-scans source for the full
nimiq-style.icons.svg#<name> literal; splitting it across a
concatenated multi-line string hides it and 404s the icon in the
deployed app. Join the arrow and info-icon <use> strings back onto
one line and suppress max-len, as done elsewhere for icon markup.
Replace the redundant `${formatNumber()}` + concatenation in the per-tx
and totals value writes with a single template literal (A7/A8), and add
the missing amount/symbol spacing as a .tx-value .nim-symbol margin to
match how .tx-total-value and .total already do it (A11), rather than a
literal space.
Wrap the per-tx and totals fee innerHTML writes in TemplateTags.hasVars(1)
so the interpolated-variable count is asserted at runtime, matching the
convention used in Iqons.js. Fold each into a single tagged template
(rather than tagged-template + string concatenation, which trips
prefer-template) and suppress max-len, and declare the TemplateTags global.
The validator address is the only validator datum the Keyguard verifies
(the new delegation is read back from the signed update-staker
newDelegation); the label and image are unverified caller data. Show the
label as the primary line but always render the address beneath it as a
left-aligned subline, never replacing the address with the label. Applied
to both validator cards for consistency. The subline reuses the shared
.address utility for its mono/ellipsis styling.
If the validator image fails synchronously (cached failure or malformed
URL), the error event can fire before a listener attached after the src
assignment exists to catch it, leaving a broken img with no identicon
fallback. Attach the error handler first, then set src.
aria-label is only exposed to screen readers, so sighted users got no
hint on hover. Add a native title attribute with the same translated
label, kept in sync on language change.
The info icon suppressed its native focus outline with outline:none,
leaving keyboard users without a visible focus indicator (unlike the
overlay close button, which keeps the native ring). Drop the outline
suppression and round the button so the native focus ring renders as a
circle around the icon.
The multi-transaction main view re-implemented the count + entry list +
totals that _buildTxListInto already builds for the tx-list overlay.
Delegate to it instead: empty the .multi-transaction template in
index.html, and generalize the .tx-* CSS from being scoped under
#tx-list-details-content to #confirm-transaction so it serves both
contexts, with the only real difference — list height — split into an
overlay rule (flex) and a main-view rule (max-height). Also translate
the entry list in _buildTxListInto so per-entry fee labels localize in
both views. Removes the duplicate .total-value rule (review comment C1).
Move the Escape keydown handler from _setupTxListOverlay (only run for
the tx-list overlay layouts) to the constructor, so Escape closes the
address-details overlay for every layout, not just switch-validator and
unstaking (C3). Remove the .focus() calls from the tx-list overlay so its
focus behaviour matches the address-details overlay; Escape-to-close now
covers keyboard users (C2).
Reformat the one-line heading show/hide rules into the project's normal
multi-line block style (C5), and collapse the per-layout back-button hide
list into a single :not(.checkout, .cashlink) selector that doesn't need
updating when a layout is added (C4).
Since validatorAddress was dropped from the switch-validator request,
the remaining omit-union fits within 120 columns on a single line.
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