Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transaction signing fixes #523

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Transaction signing fixes #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f84fa3c
Allow SignTransaction to sign HTLC redeem transactions
sisou Jan 13, 2025
8d8a8cf
Fix calculated validityStartHeight of swap NIM refund transactions
sisou Jan 13, 2025
627c170
Let the Nimiq SDK calculate the HTLC address
sisou Jan 13, 2025
7fb9b14
Use newly added Nimiq SDK enums for hardcoded values again
sisou Jan 13, 2025
54311e7
Address review comments
sisou Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/lib/RequestParser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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
Expand All @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions src/request/sign-swap/SignSwapApi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down
38 changes: 7 additions & 31 deletions src/request/sign-transaction/SignTransaction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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 ...
}
Expand Down
40 changes: 25 additions & 15 deletions src/request/swap-iframe/SwapIFrameApi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

The fee calculation needs to be updated as transaction sizes changed with PoS.

Additionally, enforcing 0 as minimum value for the transaction amount would also make sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

167 bytes for refund txs is actually accurate. No idea if I updated this at some point, or it didn't change 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

As an example see a refund tx from Fastspot: 098f74db2d247e05b3175933dbcd6027e21e36f58e06e94957f31ad14696bfd2

{
  hash: '098f74db2d247e05b3175933dbcd6027e21e36f58e06e94957f31ad14696bfd2',
  blockNumber: 6815039,
  timestamp: 1735439672089,
  confirmations: 1448037,
  size: 167,
  relatedAddresses: [
    'NQ79 JPT1 FFTJ U9TD HNRU N07T 4FGR 0PJJ VJ1D',
    'NQ56 VE9K PV0Y TK4T QBMY 8E9G B85J QR6M MSHL'
  ],
  from: 'NQ79 JPT1 FFTJ U9TD HNRU N07T 4FGR 0PJJ VJ1D',
  fromType: 2,
  to: 'NQ56 VE9K PV0Y TK4T QBMY 8E9G B85J QR6M MSHL',
  toType: 0,
  value: 8301585962,
  fee: 0,
  senderData: '',
  recipientData: '',
  flags: 0,
  validityStartHeight: 6815037,
  proof: '020008600ec9f0d44dc8d43275c705d7780caa31497d2620da4d7838d10574a6dfa100de3165b68453935067076f726018c60aaaa5ca0a128d0d0dc9882d309c63b359b2a229ca3b95b648c8abf97cf0d9fdba60440c7de57c192b9f5a23a5e59b4800',
  networkId: 24,
  executionResult: true
}

0 /* Nimiq.Transaction.Flag.NONE */,
parsedRequest.fund.htlcDetails.timeoutTimestamp,
Nimiq.TransactionFlag.None,
validityStartHeight,
CONFIG.NIMIQ_NETWORK_ID,
);

Expand Down
Loading