chore: storage method to sign tx with custom override #618

r4mmer · 2024-03-04T16:20:37Z

Acceptance Criteria

Allow the user to override the tx signature method.
Default to transactionUtils.signTransaction, our main signature method.

Security Checklist

Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

codecov · 2024-03-04T16:23:44Z

Codecov Report

Attention: Patch coverage is 97.95918% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 79.23%. Comparing base (0e6ed05) to head (f746f70).

Files	Patch %	Lines
src/new/wallet.js	96.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #618      +/-   ##
==========================================
+ Coverage   79.14%   79.23%   +0.08%     
==========================================
  Files          73       73              
  Lines        5731     5749      +18     
  Branches     1214     1217       +3     
==========================================
+ Hits         4536     4555      +19     
+ Misses       1178     1177       -1     
  Partials       17       17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

r4mmer · 2024-03-04T20:19:03Z

src/new/wallet.js

@@ -1571,7 +1571,7 @@ class HathorWallet extends EventEmitter {
        isCreateNFT: newOptions.isCreateNFT,
      },
    );
-    return await transactionUtils.prepareTransaction(
+    return transactionUtils.prepareTransaction(


note: These are just to remove unnecessary awaits.

r4mmer · 2024-03-04T20:20:30Z

src/new/wallet.js

  async getSignatures(tx, { pinCode = null } = {}) {
-    if (await this.storage.isReadonly()) {
-      throw new WalletFromXPubGuard('getSignatures');
-    }
    const pin = pinCode || this.pinCode;
-    if (!pin) {
-      throw new Error(ERROR_MESSAGE_PIN_REQUIRED);
-    }
-
-    const xprivkey = await this.storage.getMainXPrivKey(pin);
-    const key = HDPrivateKey(xprivkey);
-
-    const signatures = [];
-    const dataToSignHash = tx.getDataToSignHash();
-    for (const indexes of await this.getWalletInputInfo(tx)) {
-      const { addressIndex } = indexes;
-      // Derive key to addressIndex
-      const derivedKey = key.deriveNonCompliantChild(addressIndex);
-      const privateKey = derivedKey.privateKey;
-      // Get tx signature and populate transaction
-      const sigDER = transactionUtils.getSignature(dataToSignHash, privateKey);
-      signatures.push({
-        signature: sigDER.toString('hex'),
-        pubkey: privateKey.publicKey.toString(),
-        ...indexes,
-      });
-    }
-
-    return signatures;
+    const signatures = await this.storage.signTxP2PKH(tx, pin);
+    return signatures.map(sigData => ({
+      ...sigData,
+      addressPath: this.getAddressPathForIndex(sigData.addressIndex),
+    }));


note: This will allow us to sign transactions for P2SH wallets using either the bitcore signing from the storage mainKey or the custom signing method.

src/storage/storage.ts

r4mmer · 2024-03-04T20:25:13Z

src/storage/storage.ts

+  async getAddressPubkey(index: number): Promise<string> {
+    const addressInfo = await this.store.getAddressAtIndex(index);
+    if (addressInfo?.publicKey) {
+      // public key already cached on address info
+      return addressInfo.publicKey;
+    }
+
+    // derive public key from xpub
+    const accessData = await this._getValidAccessData();
+    const hdpubkey = new HDPublicKey(accessData.xpubkey);
+    const key: HDPublicKey = hdpubkey.deriveChild(index);
+    return key.publicKey.toString('hex');
+  }


note: We only got the pubkey from the derived mainKey, this does not work for external signing where we only have the xpubkey.

r4mmer · 2024-03-04T20:26:19Z

src/utils/transaction.ts

@@ -123,12 +123,13 @@ const transaction = {
    return signature.toDER();
  },

-  async signTransaction(tx: Transaction, storage: IStorage, pinCode: string): Promise<Transaction> {
+  async getSignatureForTx(tx: Transaction, storage: IStorage, pinCode: string): Promise<IInputSignature[]> {


note: This new util is the default signing method for the wallet-lib for now.

tuliomir

thought(blocking): It troubles me that we're changing a central protection ( read only guards ) and removing the tests that would assure us it's working as intended later.

I'd suggest splitting this feature differently:

First with a refactor for the transaction utils with all new signature methods. All tests would have to be green still
Then adding a flag to allow for a full wallet with external cryptography ( the scope of feat: allow disabling the xpub guard #619 , as I understand ). The tests could be adapted accordingly, with almost no new tests needed.
Lastly, implement the feature to actually initialize the wallet without any xPriv and having an external signing method. New tests would be made for this new feature.

What do you think of this approach?

src/new/wallet.js

src/storage/storage.ts

tuliomir · 2024-03-06T21:01:11Z

src/new/wallet.js

@@ -515,7 +522,7 @@ class HathorWallet extends EventEmitter {
          sigs.push(hexToBuffer(p2shSig.signatures[index]));
        } catch (e) {
          // skip if there is no signature, or if it's not hex
-          continue
+          continue;


note(non-blocking): Codecov is raising one case that was not tested: having invalid p2sh signatures.

Since P2SH transactions involve message exchange and direct user input I think it's something actually plausible to happen. Maybe we could open an issue to remind us of this test case.

Can you create an issue for this?
I think its important, but it is beyond the scope of this PR.

Opened #623 requesting this test

tuliomir · 2024-03-06T21:05:29Z

src/storage/storage.ts

+   * @param {EcdsaTxSign} txSign The signing function
+   */
+  setTxSign(txSign: EcdsaTxSign): void {
+    this.txSignFunc = txSign;


question: Would it be too much of an effort to create a txSignFunc and test it? The best case would be on an integration test, but I believe just a unit test would be enough for now.

That way we would ensure the override process is actually working.

getSignatureForTx is the perfect example, it fetches/derives the mainKey and uses the storage to iterate on the inputs that are owned by the wallet, signing only those that belong to the wallet.

Any other test method would essentially need to have to do the same.
During tests what I would do is create a function that prints something to stdout and call getSignatureForTx, so I would know it called the correct method and that it works as expected.

I'm not sure if I understand: getSignatureForTx is an example of signing function - actually the default one used by the lib, right?

CoveCov is informing that we have not tested this line 92. As I understand, to increase this coverage in unit tests we would just need to call setTxSign with a mocked function and ensure it's actually invoked whenever a signature is required within the scope of this storage/wallet.

Integration tests for an actual valid external signature could be left to another moment.

Created an integration test showcasing this.
It will also increase the coverage since it tests the custom signature method case.

tuliomir · 2024-03-06T21:07:06Z

src/storage/storage.ts

+   * @returns {Promise<string>} The public key DER encoded in hex
+   */
+  async getAddressPubkey(index: number): Promise<string> {
+    const addressInfo = await this.store.getAddressAtIndex(index);


chore: We have no coverage on the new getAddressPubkey method. I feel this is important to validate the new feature.

Added tests

tuliomir · 2024-03-06T21:10:45Z

src/storage/storage.ts

+   * @param {string} pinCode The pin code
+   * @returns {Promise<IInputSignature[]>} The signatures
+   */
+  async signTx(tx: Transaction, pinCode: string): Promise<IInputSignature[]> {


suggestion: This method does not sign the transaction, but only returns an array of signatures for another method to inject it into the Transaction object.

I'd suggest renaming this method to getTxSignatures(tx, pinCode) to better reflect that it is a pure function.

src/types.ts

src/storage/storage.ts

src/utils/transaction.ts

src/storage/storage.ts

src/new/wallet.js

r4mmer requested a review from tuliomir March 4, 2024 16:20

r4mmer self-assigned this Mar 4, 2024

r4mmer requested a review from pedroferreira1 as a code owner March 4, 2024 16:20

r4mmer commented Mar 4, 2024

View reviewed changes

src/storage/storage.ts Outdated Show resolved Hide resolved

r4mmer commented Mar 4, 2024

View reviewed changes

tuliomir reviewed Mar 5, 2024

View reviewed changes

src/new/wallet.js Outdated Show resolved Hide resolved

src/storage/storage.ts Show resolved Hide resolved

src/storage/storage.ts Show resolved Hide resolved

src/storage/storage.ts Outdated Show resolved Hide resolved

tuliomir reviewed Mar 6, 2024

View reviewed changes

pedroferreira1 reviewed Mar 7, 2024

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

src/storage/storage.ts Outdated Show resolved Hide resolved

src/utils/transaction.ts Show resolved Hide resolved

src/storage/storage.ts Show resolved Hide resolved

r4mmer added 17 commits March 14, 2024 11:09

chore: storage method to sign tx with custom override

e058dac

chore: custom signature does not mount the transaction

e11c721

refactor: rename method that is not P2PKH-only anymore

548a1b0

tests: correctly mock getSignatures deps

d7c048c

tests(integration): disable xpub guard on signature method

1bdfc2b

feat: p2sh signatures should be hex encoded

a66b1b4

feat: pubkey should be hex encoded on getSignatures

d1fa03f

docs: docstrings for new methods

a986701

feat: allow disabling the xpub guard

72a55d4

tests(integration) increase coverage

89f9c12

tests(integration) change test placement

7129429

tests(integration) rename disable xpubguard option

1d68562

tests: getAddressPubkey test

5597d82

chore: rename signTx storage method

28830f9

chore: fix docstring

217c250

tests: fix test mock

905b19b

tests(integration): bind mocked util

dd9d4dd

r4mmer force-pushed the feat/custom-signature-method branch from df0e80d to dd9d4dd Compare March 14, 2024 15:37

r4mmer requested review from pedroferreira1 and tuliomir March 14, 2024 17:07

pedroferreira1 reviewed Mar 15, 2024

View reviewed changes

src/new/wallet.js Show resolved Hide resolved

r4mmer assigned tuliomir and andreabadesso and unassigned tuliomir and andreabadesso Mar 15, 2024

r4mmer requested a review from andreabadesso March 15, 2024 15:19

andreabadesso approved these changes Mar 15, 2024

View reviewed changes

chore: review requests

6130ed3

r4mmer requested a review from pedroferreira1 March 18, 2024 17:48

pedroferreira1 approved these changes Mar 18, 2024

View reviewed changes

r4mmer added 3 commits March 18, 2024 15:20

tests(integration): use new storage methods

485ec08

tests(integration): fix storage not found

bcbc9af

Merge branch 'dev' into feat/custom-signature-method

f746f70

r4mmer merged commit 50bb6ea into dev Mar 18, 2024
6 checks passed

r4mmer deleted the feat/custom-signature-method branch March 18, 2024 20:52

tuliomir mentioned this pull request Mar 20, 2024

test: Invalid p2sh signatures #623

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: storage method to sign tx with custom override #618

chore: storage method to sign tx with custom override #618

r4mmer commented Mar 4, 2024

codecov bot commented Mar 4, 2024 •

edited

Loading

r4mmer Mar 4, 2024

r4mmer Mar 4, 2024

r4mmer Mar 4, 2024

r4mmer Mar 4, 2024

tuliomir left a comment

tuliomir Mar 6, 2024

r4mmer Mar 14, 2024

tuliomir Mar 20, 2024 •

edited

Loading

tuliomir Mar 6, 2024

r4mmer Mar 7, 2024

tuliomir Mar 9, 2024

r4mmer Mar 14, 2024

tuliomir Mar 6, 2024

r4mmer Mar 14, 2024

tuliomir Mar 6, 2024

r4mmer Mar 14, 2024

chore: storage method to sign tx with custom override #618

chore: storage method to sign tx with custom override #618

Conversation

r4mmer commented Mar 4, 2024

Acceptance Criteria

Security Checklist

codecov bot commented Mar 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliomir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliomir Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2024 •

edited

Loading

tuliomir Mar 20, 2024 •

edited

Loading