Multi-Transaction Signing for SignTransaction#551
Conversation
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
left a comment
danimoh
left a comment
There was a problem hiding this comment.
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.
| export type TransactionInfo = { | ||
| keyPath: string, | ||
| senderLabel?: string, | ||
| sender: Uint8Array, | ||
| senderType: Nimiq.AccountType, | ||
| senderData?: Uint8Array, | ||
| recipient: Uint8Array, | ||
| recipientType?: Nimiq.AccountType, | ||
| recipientData?: Uint8Array, | ||
| value: number, | ||
| fee: number, | ||
| validityStartHeight: number, | ||
| flags?: number, | ||
| }; | ||
|
|
||
| // TransactionInfo without keyPath (keyPath is at request level for multi-transaction support) | ||
| export type TransactionData = Omit<TransactionInfo, 'keyPath'>; | ||
|
|
There was a problem hiding this comment.
Could also be the other way around.
Additionally, it can be considered to rename TransactionInfo to TransactionDataWithKeyPath to reduce confusion between TransactionInfo and TransactionData.
export type TransactionData = {
senderLabel?: string,
sender: Uint8Array,
senderType: Nimiq.AccountType,
senderData?: Uint8Array,
recipient: Uint8Array,
recipientType?: Nimiq.AccountType,
recipientData?: Uint8Array,
value: number,
fee: number,
validityStartHeight: number,
flags?: number,
};
export type TransactionDataWithKeyPath = TransactionData & { keyPath: string };However, TransactionData is not an ideal name either, because it also includes senderLabel, which is not part of the transaction. So maybe they should be called TransactionInfo and TransactionInfoWithKeyPath.
| type SignTransactionRequestCommon = SimpleRequest & TransactionInfo; | ||
|
|
||
| export type SignTransactionRequestStandard = SignTransactionRequestCommon & { | ||
| // Standard layout supports both single and multiple transactions | ||
| export type SignTransactionRequestStandard = SimpleRequest & { | ||
| keyPath: string, | ||
| layout?: 'standard', | ||
| recipientLabel?: string, | ||
| }; | ||
| recipientLabel?: string, // Only used for single-tx display | ||
| } & ( | ||
| // Option A: Single transaction (backward compatible - fields directly on request) | ||
| TransactionData | | ||
| // Option B: Multiple transactions | ||
| { transactions: TransactionData[] | Uint8Array[] } | ||
| ); |
There was a problem hiding this comment.
The name SignTransactionRequestCommon is not adequate anymore if it's not used in SignTransactionRequestStandard anymore, as it's then in fact not common to all SignTransactionRequests.
This can be circumvented by actually using the type here, as is still proves useful:
export type SignTransactionRequestStandard = (
SignTransactionRequestCommon
| (SimpleRequest & {
keyPath: string,
transactions: TransactionData[] | Uint8Array[],
})
) & {
layout?: 'standard',
recipientLabel?: string, // Only used for single-tx display
};Or to go further, and explicitly allow senderLabel and recipientLabel only for a single transaction:
| type SignTransactionRequestCommon = SimpleRequest & TransactionInfo; | |
| export type SignTransactionRequestStandard = SignTransactionRequestCommon & { | |
| // Standard layout supports both single and multiple transactions | |
| export type SignTransactionRequestStandard = SimpleRequest & { | |
| keyPath: string, | |
| layout?: 'standard', | |
| recipientLabel?: string, | |
| }; | |
| recipientLabel?: string, // Only used for single-tx display | |
| } & ( | |
| // Option A: Single transaction (backward compatible - fields directly on request) | |
| TransactionData | | |
| // Option B: Multiple transactions | |
| { transactions: TransactionData[] | Uint8Array[] } | |
| ); | |
| export type SignTransactionRequestStandard = (( | |
| // Legacy request type for a single transaction | |
| SignTransactionRequestCommon & { | |
| recipientLabel?: string, | |
| } | |
| ) | ( | |
| // New request type for a single transaction. senderLabel and recipientLabel are supported. | |
| SimpleRequest & { | |
| keyPath: string, | |
| recipientLabel?: string, | |
| transactions: [TransactionData] | [Uint8Array], | |
| } | |
| ) | ( | |
| // New request type for multiple transactions. senderLabel and recipientLabel are not supported | |
| SimpleRequest & { | |
| keyPath: string, | |
| transactions: Omit<TransactionData, 'senderLabel'>[] | Uint8Array[], | |
| } | |
| )) & { | |
| layout?: 'standard', | |
| }; |
| // Array result type for multi-transaction signing | ||
| export type SignTransactionResults = SignTransactionResult[]; |
There was a problem hiding this comment.
Not sure whether it's worth introducing a type alias for this. Imo, just using SignTransactionResult[] would also be fine.
| // Allow sender=recipient for staking transactions (e.g., retire stake) | ||
| if (tx.sender.equals(tx.recipient) | ||
| && tx.senderType !== Nimiq.AccountType.Staking | ||
| && tx.recipientType !== Nimiq.AccountType.Staking) { |
There was a problem hiding this comment.
I don't think that the sender and recipient can be the same even for staking transactions. One side is the staking contract, the other side the sender of funds or fees, or the recipient of funds.
| <!-- Multi-transaction view --> | ||
| <div class="page-body nq-card-body transaction multi-transaction display-none"> | ||
| <div class="transaction-summary"> | ||
| <span class="transaction-count nq-text"></span> | ||
| </div> | ||
| <div class="transaction-list"> | ||
| <!-- Transaction cards will be dynamically inserted here --> | ||
| </div> | ||
| <div class="transaction-totals"> | ||
| <div class="total-value nq-light-blue"> | ||
| <span class="total-value-amount"></span><span class="nim-symbol"></span> | ||
| </div> | ||
| <div class="total-fees nq-text-s"> | ||
| + <span class="total-fees-amount"></span> <span class="nim-symbol"></span> | ||
| <span data-i18n="sign-tx-multi-total-fees">total fees</span> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
I would move this below the Single Transaction View because that is rather the standard view.
| // Deserialize using Nimiq's fromAny method | ||
| const tx = Nimiq.Transaction.fromAny(Nimiq.BufferUtils.toHex(txBytes)); | ||
|
|
||
| // Validate transaction constraints (same as parseTransaction) |
There was a problem hiding this comment.
Can consider calling tx.verify, and check that it fails because of an invalid signature, but not because of another error.
| ? serializedTx | ||
| : new Uint8Array(Object.values(serializedTx)); | ||
|
|
||
| // Deserialize using Nimiq's fromAny method |
There was a problem hiding this comment.
Unnecessary comment.
| // Deserialize using Nimiq's fromAny method |
| parsedRequest.senderLabel = this.parseLabel(request.senderLabel); | ||
| } | ||
| } else { | ||
| // EXISTING: TransactionData object path |
There was a problem hiding this comment.
| // EXISTING: TransactionData object path | |
| // TransactionData object |
| } else { | ||
| // EXISTING: TransactionData object path | ||
| parsedRequest.transactions = request.transactions.map( | ||
| /** @param {KeyguardRequest.TransactionData} tx */ |
There was a problem hiding this comment.
If the public request type was to be updated to exclude the senderLabel (see suggestion there), it should be adapted here, too:
| /** @param {KeyguardRequest.TransactionData} tx */ | |
| /** @param {Omit<KeyguardRequest.TransactionData, 'senderLabel'>} tx */ |
|
|
||
| // For single-item arrays, extract senderLabel for backward compatibility | ||
| if (request.transactions.length === 1) { | ||
| parsedRequest.senderLabel = this.parseLabel(request.senderLabel); |
There was a problem hiding this comment.
senderLabel does not exist on the request, if it has transactions. Does typescript not flag this as a type error?
danimoh
left a comment
danimoh
left a comment
There was a problem hiding this comment.
Review of SignTransaction.js.
I haven't looked at the demo yet, and also haven't tested.
|
|
||
| const transaction = request.transaction; | ||
|
|
||
| this._isMultiTransaction = request.transactions.length > 1; |
There was a problem hiding this comment.
This is only used within this constructor, so can be a local const instead of a property on the instance.
| } | ||
|
|
||
| /** | ||
| * Renders the single transaction view (existing behavior) |
There was a problem hiding this comment.
| * Renders the single transaction view (existing behavior) |
The method name is self-explanatory.
| minLength: request.keyInfo.hasPin ? Key.PIN_LENGTH : undefined, | ||
| }); | ||
| /** | ||
| * Renders the multi-transaction view |
There was a problem hiding this comment.
| * Renders the multi-transaction view |
The method name is self-explanatory.
| // Hide single-transaction view, show multi-transaction view | ||
| const $singleTx = /** @type {HTMLElement} */ (this.$el.querySelector('.single-transaction')); | ||
| const $multiTx = /** @type {HTMLElement} */ (this.$el.querySelector('.multi-transaction')); | ||
| $singleTx.classList.add('display-none'); | ||
| $multiTx.classList.remove('display-none'); |
There was a problem hiding this comment.
Controling the display via the hide-* classes would be more elegant, without the need of additional css.
| const txCount = String(request.transactions.length); | ||
| const updateCount = () => { | ||
| $count.textContent = I18n.translatePhrase('sign-tx-multi-count').replace('{count}', txCount); | ||
| }; | ||
| updateCount(); | ||
| I18n.observer.on(I18n.Events.LANGUAGE_CHANGED, updateCount); |
There was a problem hiding this comment.
| const txCount = String(request.transactions.length); | |
| const updateCount = () => { | |
| $count.textContent = I18n.translatePhrase('sign-tx-multi-count').replace('{count}', txCount); | |
| }; | |
| updateCount(); | |
| I18n.observer.on(I18n.Events.LANGUAGE_CHANGED, updateCount); | |
| I18n.translateToHtmlContent($count, 'sign-tx-multi-count', { count: String(request.transactions.length) }); |
| // Address (short form) | ||
| const $address = document.createElement('div'); | ||
| $address.className = 'tx-address'; | ||
| $address.textContent = recipientAddress; | ||
| $details.appendChild($address); |
There was a problem hiding this comment.
The address must also be accesible in full, without text ellipsis. I suggest opening the details overlay with the full address when clicking the address / identicon, like in the single transaction view.
| // Details container | ||
| const $details = document.createElement('div'); | ||
| $details.className = 'tx-details'; |
There was a problem hiding this comment.
The transaction data must be shown. Otherwise the user might be signing a contract creation without knowing, or worse, signing an unwanted update to his validator in what looks like a harmless 0 NIM transaction.
Can use TransactionDataFormatting for this, just like in the single transaction case. It tries to display all relevant information, but this might not be in the most user-friendly fashion. Thus, it would be worth additionally building specific UIs / cards for transaction types typically to be expected here, like unstaking transactions.
| // Backward compatible: return single result for single tx, array for multiple | ||
| resolve(results.length === 1 ? results[0] : results); | ||
| } else { | ||
| // For non-staking transactions, use the existing manual signing approach |
There was a problem hiding this comment.
| // For non-staking transactions, use the existing manual signing approach | |
| // For incoming staking transactions with user-provided staker / validator signature proof and non- | |
| // staking transactions, use the manual signing approach. `transaction.sign()` does not currently support | |
| // user-provided staker / validator signature proofs or HTLC redemptions. | |
| // Note however, thas this will not return a valid HTLC redemption signature proof. It has to be built | |
| // manually from the signature. |
| // For staking transactions, use the same approach as SignStaking: | ||
| // Derive a KeyPair and call transaction.sign() |
There was a problem hiding this comment.
| // For staking transactions, use the same approach as SignStaking: | |
| // Derive a KeyPair and call transaction.sign() | |
| // For staking transactions, use `transaction.sign()` for automatically generating the staker / validator | |
| // signature proof in the recipient data. The same keypair as for signing the transaction will be used for | |
| // this. Arbitrary signature proofs for a different staker or validator address are not supported. |
| tx.senderType === Nimiq.AccountType.Staking || tx.recipientType === Nimiq.AccountType.Staking); | ||
|
|
||
| request.transaction.proof = Nimiq.SignatureProof.singleSig(publicKey, signature).serialize(); | ||
| if (hasStakingTx) { |
There was a problem hiding this comment.
Also check whether the staking transaction includes a user-provided signature proof, and defer to manual signing in that case.
Alternatively, reject staking transactions with a user-provided signature proof during parsing.
2 participants
Summary
SignTransactionrequest to accept multiple transactions (TransactionData[]orUint8Array[]) that are signed with a single password entry, enabling batch operations like combined staking + transfer flowsSignTransactionResult; only multi-item arrays returnSignTransactionResultsChanges
Client types (
client/src/PublicRequest.ts)TransactionDatatype (TransactionInfowithoutkeyPath, sincekeyPathlives at request level)SignTransactionRequestStandardnow accepts either inlineTransactionDatafields or a{ transactions: TransactionData[] | Uint8Array[] }objectSignTransactionResultsarray type;ResultTypeandResultByCommandupdated accordinglyRequest parsing (
SignTransactionApi.js)TransactionData[], orUint8Array[](serialized, for staking transactions)postMessageUint8Array→ plain object conversionInternal types (
Keyguard.d.ts)Parsedfor Standard now usestransactions: Nimiq.Transaction[]instead oftransaction: Nimiq.TransactionParsedfor Checkout and Cashlink updated withTransformto also maptransaction→transactionsUI handler (
SignTransaction.js,SignTransaction.css,index.html)_renderMultiTransactionViewand_renderSingleTransactionViewbased ontransactions.lengthI18n.observertransaction.sign(keyPair)when any staking tx is present, manualSignatureProof.singleSigotherwise#confirm-transaction.multi— no impact onSignMultisigTransactionwhich shares the same stylesheetTranslations (
en.json)sign-tx-heading-multi,sign-tx-multi-count,sign-tx-multi-total-fees,passwordbox-confirm-txsOther
PasswordBox.js: addedpasswordbox-confirm-txsbutton templateRequestParser.js: minor adjustment for sharedparseTransaction()usagedemos/SignTransaction.html: comprehensive demo with format selector and multi-tx controlsTesting
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.