Merge pull request #523 from nimiq/soeren/tx-signing-fixes

Transaction signing fixes
nimiq · Jan 15, 2025 · 4e49592 · 4e49592
2 parents 5be3829 + 54311e7
commit 4e49592
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 53 deletions.
diff --git a/src/lib/RequestParser.js b/src/lib/RequestParser.js
@@ -132,7 +132,7 @@ class RequestParser { // eslint-disable-line no-unused-vars
             Nimiq.AccountType.Basic,
             Nimiq.AccountType.Vesting,
             Nimiq.AccountType.HTLC,
-            3 /* Staking */,
+            Nimiq.AccountType.Staking,
         ]);
         if (!object || typeof object !== 'object' || object === null) {
             throw new Errors.InvalidRequestError('Request must be an object');
@@ -158,16 +158,16 @@ class RequestParser { // eslint-disable-line no-unused-vars
             ? Utf8Tools.stringToUtf8ByteArray(object.recipientData)
             : object.recipientData || new Uint8Array(0);
 
-        const flags = object.flags || 0/* Nimiq.Transaction.Flag.NONE */;
+        const flags = object.flags || Nimiq.TransactionFlag.None;
 
         if (
-            flags === 0 /* Nimiq.Transaction.Flag.NONE */
+            flags === Nimiq.TransactionFlag.None
             && recipientType !== Nimiq.AccountType.Staking
             && recipientData.byteLength > 64
         ) {
             throw new Errors.InvalidRequestError('Data must not exceed 64 bytes');
         }
-        if (flags === 1 /* Nimiq.Transaction.Flag.CONTRACT_CREATION */
+        if (flags === Nimiq.TransactionFlag.ContractCreation
                 && recipientData.byteLength !== 82 // HTLC
                 && recipientData.byteLength !== 28 // Vesting
                 && recipientData.byteLength !== 44 // Vesting
@@ -177,7 +177,7 @@ class RequestParser { // eslint-disable-line no-unused-vars
             );
         }
         if (
-            flags === 1 /* Nimiq.Transaction.Flag.CONTRACT_CREATION */
+            flags === Nimiq.TransactionFlag.ContractCreation
             && recipient !== 'CONTRACT_CREATION'
         ) {
             throw new Errors.InvalidRequestError(

diff --git a/src/request/sign-swap/SignSwapApi.js b/src/request/sign-swap/SignSwapApi.js
@@ -47,7 +47,7 @@ class SignSwapApi extends PolygonRequestParserMixin(BitcoinRequestParserMixin(To
                     // Enforced properties
                     recipient: 'CONTRACT_CREATION',
                     recipientType: Nimiq.AccountType.HTLC,
-                    flags: 1 /* Nimiq.Transaction.Flag.CONTRACT_CREATION */,
+                    flags: Nimiq.TransactionFlag.ContractCreation,
                 }),
                 senderLabel: /** @type {string} */ (this.parseLabel(
                     request.fund.senderLabel, false, 'fund.senderLabel',
@@ -127,7 +127,7 @@ class SignSwapApi extends PolygonRequestParserMixin(BitcoinRequestParserMixin(To
                     // Enforced properties
                     senderType: Nimiq.AccountType.HTLC,
                     recipientType: Nimiq.AccountType.Basic,
-                    flags: 0 /* Nimiq.Transaction.Flag.NONE */,
+                    flags: Nimiq.TransactionFlag.None,
                 }),
                 recipientLabel: /** @type {string} */ (this.parseLabel(
                     request.redeem.recipientLabel, false, 'recipientLabel',

diff --git a/src/request/sign-transaction/SignTransaction.js b/src/request/sign-transaction/SignTransaction.js
@@ -182,40 +182,16 @@ class SignTransaction {
             return;
         }
 
-        const privateKey = key.derivePrivateKey(request.keyPath);
-        const keyPair = Nimiq.KeyPair.derive(privateKey);
-
-        /** @type {Nimiq.Transaction} */
-        let tx;
-
-        if (!request.transaction.data.length) {
-            tx = Nimiq.TransactionBuilder.newBasic(
-                keyPair.toAddress(),
-                Nimiq.Address.fromString(request.transaction.recipient.toHex()),
-                BigInt(request.transaction.value),
-                BigInt(request.transaction.fee),
-                request.transaction.validityStartHeight,
-                request.transaction.networkId,
-            );
-        } else {
-            tx = Nimiq.TransactionBuilder.newBasicWithData(
-                keyPair.toAddress(),
-                Nimiq.Address.fromString(request.transaction.recipient.toHex()),
-                request.transaction.data,
-                BigInt(request.transaction.value),
-                BigInt(request.transaction.fee),
-                request.transaction.validityStartHeight,
-                request.transaction.networkId,
-            );
-        }
+        const publicKey = key.derivePublicKey(request.keyPath);
+        const signature = key.sign(request.keyPath, request.transaction.serializeContent());
 
-        tx.sign(keyPair);
+        request.transaction.proof = Nimiq.SignatureProof.singleSig(publicKey, signature).serialize();
 
         /** @type {KeyguardRequest.SignTransactionResult} */
         const result = {
-            publicKey: keyPair.publicKey.serialize(),
-            signature: tx.proof.subarray(tx.proof.length - 64),
-            serializedTx: tx.serialize(),
+            publicKey: publicKey.serialize(),
+            signature: signature.serialize(),
+            serializedTx: request.transaction.serialize(),
         };
         resolve(result);
     }
@@ -234,7 +210,7 @@ class SignTransaction {
             return I18n.translatePhrase('funding-cashlink');
         }
 
-        if (transaction.flags === 1 /* Nimiq.Transaction.Flag.CONTRACT_CREATION */) {
+        if (transaction.flags === Nimiq.TransactionFlag.ContractCreation) {
             // TODO: Decode contract creation transactions
             // return ...
         }

diff --git a/src/request/swap-iframe/SwapIFrameApi.js b/src/request/swap-iframe/SwapIFrameApi.js
@@ -409,21 +409,10 @@ class SwapIFrameApi extends BitcoinRequestParserMixin(RequestParser) { // eslint
             const privateKey = new Nimiq.PrivateKey(Nimiq.BufferUtils.fromHex(privateKeys.nim));
             const publicKey = Nimiq.PublicKey.derive(privateKey);
 
-            let transaction = Nimiq.Transaction.fromPlain({
-                ...storedRequest.fund.transaction.toPlain(),
-                data: {
-                    type: 'raw',
-                    raw: Nimiq.BufferUtils.toHex(parsedRequest.fund.htlcData),
-                },
-                // This NULL-address as the recipient gets replaced below
-                recipient: new Nimiq.Address(new Uint8Array(20)).toUserFriendlyAddress(),
-            });
-            // Calculate the contract address of the HTLC that gets created and recreate the transaction
-            // with that address as the recipient:
-            const contractAddress = new Nimiq.Address(Nimiq.BufferUtils.fromHex(transaction.hash()));
+            let transaction = storedRequest.fund.transaction;
             transaction = new Nimiq.Transaction(
                 transaction.sender, transaction.senderType, transaction.senderData,
-                contractAddress, transaction.recipientType, transaction.data,
+                new Nimiq.Address(new Uint8Array(20)), transaction.recipientType, parsedRequest.fund.htlcData,
                 transaction.value, transaction.fee,
                 transaction.flags, transaction.validityStartHeight, transaction.networkId,
             );
@@ -440,13 +429,34 @@ class SwapIFrameApi extends BitcoinRequestParserMixin(RequestParser) { // eslint
                 const feePerUnit = Number(transaction.fee) / transaction.serializedSize;
                 const fee = BigInt(Math.ceil(feePerUnit * 167)); // 167 = NIM HTLC refunding tx size
 
+                /**
+                 * Calculate future ValidityStartHeight for the refund transaction.
+                 *
+                 * This is not exact, unfortunately. But hopefully close enough. The calculated VSH is always
+                 * smaller than it should be (blocks are faster than 1/second on average, time taken between
+                 * request creation and this code is subtracted as well). Additionally, if the user's system
+                 * clock is off, the resulting VHS will be wrong, too.
+                 *
+                 * But the watchtower only sends the refund transaction once the timeout _timestamp_ of the swap
+                 * has passed, at which point the transaction will still be valid (except if totally wrong because
+                 * of system clock).
+                 *
+                 * If the refund transaction is invalid when it is supposed to be sent, the user can always
+                 * trigger a refund manually from the transaction details in the Wallet.
+                 */
+                const validityStartHeight = Math.max(
+                    transaction.validityStartHeight,
+                    transaction.validityStartHeight
+                        + (parsedRequest.fund.htlcDetails.timeoutTimestamp - Math.round(Date.now() / 1e3)),
+                );
+
                 // Create refund transaction
                 const refundTransaction = new Nimiq.Transaction(
                     transaction.recipient, Nimiq.AccountType.HTLC, new Uint8Array(0),
                     transaction.sender, Nimiq.AccountType.Basic, new Uint8Array(0),
                     transaction.value - fee, fee,
-                    0 /* Nimiq.Transaction.Flag.NONE */,
-                    parsedRequest.fund.htlcDetails.timeoutTimestamp,
+                    Nimiq.TransactionFlag.None,
+                    validityStartHeight,
                     CONFIG.NIMIQ_NETWORK_ID,
                 );