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

Jump to bottom

feat(root): implement Eddsa multisig derivation #4276

Closed
wants to merge 1 commit into from
Closed

feat(root): implement Eddsa multisig derivation #4276

wants to merge 1 commit into from

Conversation

alebusse
Copy link
Contributor

@alebusse alebusse commented Feb 12, 2024 •
edited
Loading

Implemented Eddsa multisig derivation

BREAKING CHANGE: wallet.getPrv() is now an async method, coin.generateKeyPair(),
coin.deriveKeyWithSeed() and keychains.create() are deprecated, use their Async version instead.

WP-1401

TICKET: WP-1401

@alebusse alebusse force-pushed the WP-1401-implement-Eddsa-multisig-derivation branch 3 times, most recently from cec8b1d to bdb5df4 Compare February 12, 2024 17:25
@alebusse alebusse marked this pull request as ready for review February 12, 2024 18:03
@alebusse alebusse requested review from a team as code owners February 12, 2024 18:03
@alebusse alebusse changed the title Wp 1401 implement eddsa multisig derivation feat(root): implement Eddsa multisig derivation Feb 12, 2024
@alebusse alebusse force-pushed the WP-1401-implement-Eddsa-multisig-derivation branch 4 times, most recently from e8e8842 to a409eeb Compare February 12, 2024 22:50
@BitGo BitGo deleted a comment from socket-security bot Feb 12, 2024
@alebusse alebusse force-pushed the WP-1401-implement-Eddsa-multisig-derivation branch from a409eeb to 9b03db8 Compare February 12, 2024 23:07
@alebusse alebusse changed the base branch from master to WP-1413-lock-socks-version-to-fix-vulnerability February 12, 2024 23:11
@alebusse alebusse changed the base branch from WP-1413-lock-socks-version-to-fix-vulnerability to master February 12, 2024 23:11
@alebusse alebusse marked this pull request as draft February 12, 2024 23:12
OttoAllmendinger
OttoAllmendinger reviewed Feb 13, 2024
View reviewed changes
Copy link
Contributor

@OttoAllmendinger OttoAllmendinger left a comment

Choose a reason for hiding this comment

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

flush draft

@alebusse
feat(root): implement Eddsa multisig derivation
3a8f9a4 
Implemented Eddsa multisig derivation

BREAKING CHANGE: wallet.getPrv() is now an async method, coin.generateKeyPair(),
coin.deriveKeyWithSeed() and keychains.create() are deprecated, use their Async version instead.

WP-1401

TICKET: WP-1401
@alebusse alebusse force-pushed the WP-1401-implement-Eddsa-multisig-derivation branch from 9b03db8 to 3a8f9a4 Compare February 13, 2024 15:17
@alebusse alebusse marked this pull request as ready for review February 13, 2024 15:47
@alebusse alebusse marked this pull request as draft February 14, 2024 14:59
mmcshinsky-bitgo
mmcshinsky-bitgo requested changes Feb 15, 2024
View reviewed changes
Copy link
Contributor

@mmcshinsky-bitgo mmcshinsky-bitgo left a comment

Choose a reason for hiding this comment

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

@alebusse BREAKING CHANGE needs to be in the commit footer to be recognized correctly

modules/sdk-core/src/account-lib/index.ts
@@ -5,5 +5,7 @@ export * from './mpc';
export * from './util/crypto';
// Deprecated
export * as acountLibCrypto from './util/crypto';
// Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant comment

modules/sdk-core/src/account-lib/util/eddsaKeyDeriver.ts
pub: string;
};

export class EddsaKeyDeriver {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs JS Docs for methods

@mmcshinsky-bitgo
Copy link
Contributor

@alebusse There are modules that are only test that aren't part of the breaking change for the module itself. This PR needs to be broken up into multiple commits so that the breaking change doesn't wrongfully bump incorrect packages.

@alebusse
Copy link
Contributor Author

@alebusse BREAKING CHANGE needs to be in the commit footer to be recognized correctly

good to know, i didnt manually created the commmit msg, commitizen cli does it for me, i follow the steps.

@alebusse
Copy link
Contributor Author

@alebusse There are modules that are only test that aren't part of the breaking change for the module itself. This PR needs to be broken up into multiple commits so that the breaking change doesn't wrongfully bump incorrect packages.

Make sense, this is not actually a PR to merge or review, is just a POC for Islam to review and work on the missing part. But i think we wont even be doing this now, im testing something else

@alebusse alebusse closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OttoAllmendinger OttoAllmendinger OttoAllmendinger left review comments

@mmcshinsky-bitgo mmcshinsky-bitgo mmcshinsky-bitgo requested changes

@islamaminBitGo islamaminBitGo Awaiting requested review from islamaminBitGo

Assignees
No one assigned
Labels
DO NOT MERGE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alebusse @mmcshinsky-bitgo @OttoAllmendinger