Transaction signing fixes #523
Conversation
sisou
force-pushed
the
soeren/tx-signing-fixes
branch
from
340dc77 to
cb2f92c
Compare
| const proof = Nimiq.SignatureProof.singleSig(publicKey, signature); | ||
| tx.proof = proof.serialize(); |
There was a problem hiding this comment.
A single sig proof is a sensible default, but one could also argue that the Keyguard should not even make any assumptions about what proof to create and only return the signature. That was also the case before bb57646.
There was a problem hiding this comment.
You are right. But the serializedTx return field is now used in Hub and probably Wallet, so removing that again will be a separate task.
| @@ -445,8 +445,9 @@ class SwapIFrameApi extends BitcoinRequestParserMixin(RequestParser) { // eslint | |||
| transaction.recipient, Nimiq.AccountType.HTLC, new Uint8Array(0), | |||
| transaction.sender, Nimiq.AccountType.Basic, new Uint8Array(0), | |||
| transaction.value - fee, fee, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
167 bytes for refund txs is actually accurate. No idea if I updated this at some point, or it didn't change 🤷
There was a problem hiding this comment.
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
}
|
Thanks @danimoh, I addressed your comments. |
There was a problem hiding this comment.
Looks good to me 👍
I was wondering at first why you replaced the contractAddress by new Nimiq.Address(new Uint8Array(20)) in SwapIFrameApi.js but @danimoh explained it to me and we went to check it in the web client, and it looks good 😁
Which is used by the manual refund flow for failed NIM swaps. Before this change, only _basic_ transactions were supported by SignTransaction, but a HTLC refund tx is an extended transaction from a HTLC account type, which was not covered by the cases. This changes it to use the already-parsed `transaction` from the request object, which allows signing of all transactions that the request accepts in the first place. We also don't worry about any proof prefix bytes here, as those are added in the Hub when the result of this Keyguard request is received. The signature will still be correct, because a signature is not made over the proof prefix bytes.
ValidityStartHeight is still a block number in PoS, not a timestamp. 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). But the watchtower only send the refund transaction once the timeout _timestamp_ of the swap has passed, at which point the transaction will still be valid.
Instead of manually calculating it, we can use the built-in logic for HTLC creation transactions. We still need to recreate the parsed `transaction` though, because the recipient data is only known here and is part of the transaction hash (from which the HTLC address is calculated).
- Simplify SignTransaction result creation (more in line with how it was before the switch to PoS) - Explain swap refund transaction validity-start-height calculation drawbacks and lower-bound it to the HTLC creation VSH
sisou
force-pushed
the
soeren/tx-signing-fixes
branch
from
f9c5d36 to
54311e7
Compare
|
Rebased for merging |
See the individual commit descriptions for change details.