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

feat(abstract-utxo): update the keySignature verification network #3697

Merged
merged 2 commits into from
Jun 30, 2023

Conversation

rushilbg
Copy link
Contributor

@rushilbg rushilbg commented Jun 28, 2023

@rushilbg rushilbg force-pushed the zec-sig branch 2 times, most recently from a3f1219 to 00ffc21 Compare June 28, 2023 07:15
@rushilbg rushilbg changed the title w feat(abstract-utxo): update the keySignature verification network Jun 28, 2023
@rushilbg rushilbg marked this pull request as ready for review June 28, 2023 10:49
@rushilbg rushilbg requested review from a team as code owners June 28, 2023 10:49
Copy link
Contributor

@brandonblack brandonblack left a comment

Choose a reason for hiding this comment

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

Please update the commit messages in this PR to include an imperative mood statement of what changed, and why.

Example:

fix(abstract-utxo): always use bitcoin network for validating secondary key signatures

Key signatures are always generated with the bitcoin network because it is not always known which coin they will be used with. Without this change altcoin signatures would fail verification in some contexts.

For a rousing review of commit messages check out my recent brown bag talk :-D

@rushilbg rushilbg requested a review from a team as a code owner June 28, 2023 13:46
@rushilbg rushilbg force-pushed the zec-sig branch 2 times, most recently from 9536c29 to 99a1ff7 Compare June 30, 2023 05:13
@rushilbg rushilbg requested a review from a team as a code owner June 30, 2023 05:13
@rushilbg rushilbg marked this pull request as draft June 30, 2023 05:13
…ary key signatures

Key signatures are always generated with the bitcoin network because it is not always
known which coin they will be used with. Without this change altcoin signatures would
fail verification in some contexts.

BG-79423
@rushilbg rushilbg force-pushed the zec-sig branch 2 times, most recently from 1dc91aa to 0495d9a Compare June 30, 2023 05:29
@rushilbg rushilbg marked this pull request as ready for review June 30, 2023 05:57
sachushaji
sachushaji previously approved these changes Jun 30, 2023
Comment on lines +10 to +12
const userKeychain = await coin.keychains().create();
const backupKeychain = await coin.keychains().create();
const bitgoKeychain = await coin.keychains().create();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const userKeychain = await coin.keychains().create();
const backupKeychain = await coin.keychains().create();
const bitgoKeychain = await coin.keychains().create();
const [ userKeychain, backupKeychain, bitgoKeychain] = await Promise.all([
coin.keychains().create(),
coin.keychains().create(),
coin.keychains().create()
]);

NIT: we can use promise.all instead of serially creating keychains

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe we are making http requests here anyway though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are synchronous operations, Promise.all won't speed them up.

@rushilbg rushilbg dismissed stale reviews from brandonblack and OttoAllmendinger June 30, 2023 07:58

Addressed

@rushilbg rushilbg requested a review from a team June 30, 2023 08:08
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.

nits

modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts Outdated Show resolved Hide resolved
modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts Outdated Show resolved Hide resolved
modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts Outdated Show resolved Hide resolved
modules/abstract-utxo/src/abstractUtxoCoin.ts Show resolved Hide resolved
@rushilbg rushilbg merged commit 0da145f into master Jun 30, 2023
5 checks passed
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.

4 participants