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.

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

WIP: generalize multisig wallet config interface #44

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bucko13
Copy link
Contributor

@bucko13 bucko13 commented Oct 5, 2020

An initial pass to see what the config interface could look like to be generalized to support multiple different config formats. This still only supports ColdCard but you should be able to see how more formats should be easily added on.

I also split out some of the validation and instantiations. I think this makes the API a little more clear and also the unit testing a little more fine grained (though maybe this isn't desired).

TODO:

  • Move existing functionality into its own config.js

  • Update tests for existing functionality

  • Replace existing functionality from coldcard.js

  • Add explicit caravan interfaces

  • Add interface to convert from a ColdCard config

  • Update jsdoc strings

@bucko13 bucko13 requested a review from humanumbrella October 5, 2020 20:16
@bucko13 bucko13 marked this pull request as draft October 5, 2020 20:16
@bucko13 bucko13 marked this pull request as ready for review October 6, 2020 00:06
@bucko13 bucko13 changed the title WIP: generalize multisig wallet config interface (RFC) generalize multisig wallet config interface Oct 6, 2020
Copy link
Contributor

@humanumbrella humanumbrella left a comment

Choose a reason for hiding this comment

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

missing some ; at the end of lines - not sure if we care.

couple questions and some nits - we can discuss soon

src/config.js Outdated Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.test.js Outdated Show resolved Hide resolved
src/config.test.js Outdated Show resolved Hide resolved
@bucko13
Copy link
Contributor Author

bucko13 commented Oct 16, 2020

@humanumbrella ready for another review. I tried to catch the semi-colons where I could. That should really be getting handled by the linter...

@bucko13 bucko13 requested a review from humanumbrella October 30, 2020 21:06
@bucko13
Copy link
Contributor Author

bucko13 commented Nov 30, 2020

Noting some requested changes here:

 if (client) {
      assert(
        this.supportedClients.includes(client.type),
        `Client type "${client.type}" not supported`
      )
      this.client = client;
    }

instead of

if (client) {
      assert(
        this.supportedClients.includes(client),
        `Client type "${client}" not supported`
      )
      this.client = client;
    }
* @param {Object} [options.client] - private or public, really only valid for caravan

instead of

* @param {string} [options.client] - private or public, really only valid for caravan

@bucko13
Copy link
Contributor Author

bucko13 commented Nov 30, 2020

@humanumbrella feedback addressed.

@bucko13 bucko13 changed the title (RFC) generalize multisig wallet config interface generalize multisig wallet config interface Dec 7, 2020
src/index.js Show resolved Hide resolved
Copy link
Contributor

@humanumbrella humanumbrella left a comment

Choose a reason for hiding this comment

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

Just some small notes and changes requested (bringing in the changes from #49 will be important)

src/coldcard.fixtures.js Outdated Show resolved Hide resolved
src/coldcard.fixtures.js Outdated Show resolved Hide resolved
src/coldcard.js Outdated Show resolved Hide resolved
src/coldcard.js Outdated Show resolved Hide resolved
* ]
* }
*/
export function parseColdcardConfig(configString) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i didnt compare line by line but this would be good to compare how they do it vs how you've done it
https://github.com/Coldcard/firmware/blob/master/shared/multisig.py#L523

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good reference. Thanks!

src/coldcard.js Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.js Show resolved Hide resolved
src/fixtures.js Outdated Show resolved Hide resolved
src/coldcard.js Outdated Show resolved Hide resolved
add support for converting from ColdCard Config

caravan interfaces

refactor coldcard config methods to be functional and other fixes:
* updates tests
* updates linter
* fixes some linting errors
src/config.js Outdated Show resolved Hide resolved
src/config.js Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.js Outdated Show resolved Hide resolved
src/config.js Show resolved Hide resolved
@@ -394,9 +394,7 @@ export function ConfirmMultisigAddress({keystore, network, bip32Path, multisig,
export function ConfigAdapter({ KEYSTORE, jsonConfig }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigAdapter -> MultisigWalletConfig
KEYSTORE -> keystore XOR coordinator or something like target (?)
jsonConfig -> braids or braidSet
add options

@bucko13 bucko13 changed the title generalize multisig wallet config interface WIP: generalize multisig wallet config interface Feb 9, 2021
@bucko13 bucko13 marked this pull request as draft February 9, 2021 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants