Adds descriptor module with getdescriptorinfo rpc #1152

Vasu-08 · 2023-05-22T17:27:09Z

This pr implements getdescriptorinfo rpc by adding a descriptor module.

If the argument string of getdescriptorinfo() rpc is a valid string it is parsed into a AbstractDescriptor object.
Along with that various parts of the string, such as origin information, keys (Public/Private), and derivation path (along with hardened or unhardened information) are parsed into PubKeyProvider object respectively. If the descriiptor string contains a checksum it will be validiated and if a checksum is not present we will calculate one.

Note: The input descriptor string doesn't require checksum to be present.

The AbstractDescriptor class has PKDescriptor, PKHDescriptor, WPKHDescriptor, AddressDescriptor, MultisigDescriptor, WSHDescriptor, SHDescriptor, RawDescriptor, ComboDescriptor as its subclasses.

The KeyProvider class will have ConstPubkeyProvider (for parsing constant keys with/without origin info) and HDKeyProvider (for parsing extended (hd - bip32) keys with/without origin info) as its subclasses.

Functions for parsing descriptor string will be present as global function in parser.js.
Checksum functions are implemented in common.js.

Functions checking isRange(), isSolvable(), hasPrivateKeys() etc. will be present in the AbstractDescriptor class.

codecov-commenter · 2023-05-22T17:38:22Z

Codecov Report

Patch coverage: 90.93% and project coverage change: +0.69 🎉

Comparison is base (bb0375e) 69.56% compared to head (5b09c3f) 70.26%.

❗ Current head 5b09c3f differs from pull request most recent head f388594. Consider uploading reports for the commit f388594 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1152      +/-   ##
==========================================
+ Coverage   69.56%   70.26%   +0.69%     
==========================================
  Files         159      174      +15     
  Lines       26607    27367     +760     
==========================================
+ Hits        18509    19229     +720     
- Misses       8098     8138      +40

Impacted Files	Coverage Δ
lib/bcoin.js	`0.00% <0.00%> (ø)`
lib/descriptor/index.js	`0.00% <0.00%> (ø)`
lib/descriptor/type/combo.js	`89.65% <89.65%> (ø)`
lib/descriptor/type/addr.js	`90.90% <90.90%> (ø)`
lib/hd/keyorigininfo.js	`92.00% <92.00%> (ø)`
lib/descriptor/abstractdescriptor.js	`92.06% <92.06%> (ø)`
lib/descriptor/type/pk.js	`92.30% <92.30%> (ø)`
lib/descriptor/type/sh.js	`92.30% <92.30%> (ø)`
lib/node/rpc.js	`41.86% <92.30%> (+0.47%)`	⬆️
lib/descriptor/type/pkh.js	`92.85% <92.85%> (ø)`
... and 9 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

lib/hd/keyorigin.js

lib/wallet/descriptor.js

lib/wallet/rpc.js

lib/utils/stringparsing.js

lib/wallet/descriptor.js

lib/descriptor/descriptor.js

lib/descriptor/common.js

pinheadmz

code review ACK 2bb70a0

The commits look super clean now THANK YOU, this was very easy to read and review. I left quite a few nits and questions below. I think everything is looking really good, just some clean up and some ideas about making your base classes more abstract.

While you address those comments I'm going to review the test vectors more closely and then play with the actual RPC feature and library functions and try to break them ;-)

GREAT WORK, keep it up! Almost done. <3

pinheadmz · 2023-07-04T12:00:57Z

lib/hd/keyorigininfo.js

+  fromOptions(options) {
+    assert(options, 'requires options');
+
+    if (options.fingerPrint !== null) {


I think these need to be != so that undefined and null are treated the same

pinheadmz · 2023-07-04T12:01:20Z

lib/hd/common.js

 * @param {Boolean} hard
- * @returns {Number[]}
+ * @returns {Array}


Why change this? isn't Number[] correct?

pinheadmz · 2023-07-04T23:03:15Z

lib/hd/keyorigininfo.js

+    const fingerPrint = isValidFingerPrint(slashSplit[0]);
+    this.fingerPrint = fingerPrint;


could just be...

Suggested change

const fingerPrint = isValidFingerPrint(slashSplit[0]);

this.fingerPrint = fingerPrint;

this.fingerPrint = isValidFingerPrint(slashSplit[0]);

pinheadmz · 2023-07-04T23:03:51Z

lib/hd/keyorigininfo.js

+ * @returns {Number}
+ */
+
+function isValidFingerPrint(str) {


maybe call this validateFingerprint() ? since it doesn't return a boolean like most is...() functions

pinheadmz · 2023-07-04T23:14:39Z

test/keyorigininfo-test.js

+/* eslint-disable quotes */
+/* eslint-disable no-unused-vars */


nits:

I dont think you need no-unsued-vars

but you should set /* eslint-env mocha */

I understand you want to use double quotes because there's so many single-quotes in the test vectors, I think that is ok. Still looks ugly to me in the first 5 lines though

pinheadmz · 2023-07-05T15:24:23Z

lib/descriptor/type/combo.js

+
+    this.network = Network.get(options.network);
+
+    for (const keyProvider of options.keyProviders) {


didn't you just assert there will only be one of these? Same with pk() maybe others...

pinheadmz · 2023-07-05T15:28:13Z

lib/descriptor/type/multisig.js

+    super();
+    this.name = types.MULTI;
+    this.isSorted = false;
+    this.threshold = null;


I think this should initialize as 0 for type optimization ?

pinheadmz · 2023-07-05T15:36:54Z

lib/descriptor/type/raw.js

+    }
+
+    this.network = Network.get(options.network);
+    this.script = options.script;


should have init value in constructor

pinheadmz · 2023-07-05T15:46:02Z

lib/descriptor/descriptor.js

+class Descriptor {
+  /**
+   * Create a descriptor
+   * @constructor
+   */
+
+  constructor() {
+    this.name = null;
+    this.keyProviders = [];
+    this.subdescriptors = [];
+    this.network = null;
+  }


See my comment about making AbstractKeyProvider - I think more stuff can be moved to the base class, like parsing some of the common options like network. keyProviders must always be an array so that can be checked in base class, then each extended class can check those array lengths, etc.

pinheadmz · 2023-07-05T15:50:23Z

lib/node/rpc.js

@@ -2765,6 +2769,26 @@ class RPC extends RPCBase {
    };
  }

+  async getDescriptorInfo(args, help) {


lets put this function after the setloglevel implementation (up on line 2328 or so). Down here is for helper functions

pinheadmz

code review ACK e96f319

Everything looks awesome, just a few picky nits and we can land this beast!

pinheadmz · 2023-07-14T18:09:17Z

lib/hd/keyorigininfo.js

+  fromOptions(options) {
+    assert(options, 'requires options');
+
+    if (options.fingerPrint) {


Suggested change

if (options.fingerPrint) {

if (options.fingerPrint != null) {

See for example ChainOptions.fromOptions() in chain.js L2639

pinheadmz · 2023-07-14T18:21:06Z

lib/hd/common.js

 * @param {Boolean} hard
 * @returns {Number[]}
 */

-common.parsePath = function parsePath(path, hard) {
-  assert(typeof path === 'string');
+common.parsePathToArray = function parsePathToArray(path, hard) {


Should this be parsePathFromArray() ?

Actually this function parses path from String so maybe I can rename it to parsePathFromString?

does it?

assert(Array.isArray(path), 'Path must be an array.');

Ohh sorry I thought you were mentioning parsePath function. I'll change parsePathToArray to parsePathFromArray

pinheadmz · 2023-07-14T18:26:25Z

lib/hd/keyorigininfo.js

+  toJSON() {
+    return {
+      fingerPrint: this.format().fingerPrint,
+      path: this.format().path
+    };
+  }


should this just return this.format()? And so only call it once?

Yes no need. Good catch.

pinheadmz · 2023-07-14T18:30:14Z

lib/descriptor/index.js

@@ -0,0 +1,27 @@
+/*!


Lets add a link to this module in lib/bcoin.js as well

Yeah should have done this way earlier. Thanks for pointing this out :). Also added KeyOriginInfo in HD here.

pinheadmz · 2023-07-14T18:31:20Z

lib/descriptor/common.js

+
+common.isHex = function isHex(str) {
+  assert(typeof str === 'string');
+  return /^[0-9a-fA-F]+$/.test(str);


This should also probably ensure an even number of characters

I'm a bit unsure whether this function should exist. I only found 1 usage in keyprovider.js L87. I think we can remove this function from here.

I think we can have it for now . People could use it instead of calling the external dependencies. (base16)

pinheadmz · 2023-07-14T18:37:35Z

lib/descriptor/keyprovider.js

+  parseOptions(options) {
+    this.ring = options.ring;
+    this.pos = options.pos;
+    this.originInfo = options.originInfo;
+    this.hardenedMarker = options.hardenedMarker;
+    this.network = Network.get(options.network);
+  }


Don't we still need to do input validation here of the passed-in options?

I don't think it's required to do that here since we are already parsing and validating the inputs by the parsing functions of KeyProvider class.

pinheadmz · 2023-07-14T18:52:25Z

lib/descriptor/keyprovider.js

+  isRange() {
+    return false;
+  }


If these functions all return null because direct instances of the abstract KeyProvider class shouldn't be used, these methods should probably throw errors. See the methods in blockstore/abstract.js

pinheadmz · 2023-07-14T18:53:25Z

test/descriptor-test.js

+  options.script = desc.script;
+  options.network = desc.network;
+
+  switch (type) {


You already have parser.parse() is there a reason to implement that again here?

parser.parse() parses descriptor only when it is in string form. To parse descriptor from options object this function is needed.

pinheadmz · 2023-07-14T18:57:37Z

lib/descriptor/type/addr.js

+    if (options.subdescriptors) {
+      assert(options.subdescriptors.length === 0);
+    }
+
+    if (options.keyProviders) {
+      assert(options.keyProviders.length === 0);
+    }


I think you can drop the if's and just assert. Because of the abstractdescriptor constructor, we know those arrays will exist, we just need to ensure that they are empty.

pinheadmz · 2023-07-14T18:58:51Z

lib/descriptor/abstractdescriptor.js

+   */
+
+  constructor() {
+    this.name = null;


this should be this.type in all the classes, sorry I shouldve caught that sooner

lib/descriptor/parser.js

theanmolsharma

ACK e94d8cb
Excellent work on the PR! Just a few small nits to fix before we can merge.

lib/descriptor/common.js

lib/descriptor/keyprovider.js

lib/descriptor/parser.js

lib/hd/keyorigininfo.js

lib/descriptor/type/addr.js

lib/descriptor/type/multisig.js

test/descriptor-test.js

theanmolsharma

Tested ACK 5b09c3f

…escriptor parsing logic

Vasu-08 marked this pull request as draft May 22, 2023 17:27