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

MultisigAddress & MultisigSignature Implementation #290

Merged
merged 16 commits into from
May 9, 2022
Merged

MultisigAddress & MultisigSignature Implementation #290

merged 16 commits into from
May 9, 2022

Conversation

jcronyn
Copy link
Contributor

@jcronyn jcronyn commented Apr 10, 2022 •
edited
Loading

  • Create multisig public key from other keys
  • New multisig signature from multiple public keys / signatures
  • Util: Sort public keys
  • Util: Sort signatures
  • Binary helpers
  • Additional helper functions as needed
  • Test cases:
    • Create valid multisig address
    • Invalid keys to create multisig address
    • Create valid multisig signature
    • Invalid signatures for multisig signature
    • M of N signatures pass/fail
    • Create multisig payment transaction and pass validation

Pending Testing:

  • Testnet testing pending testnet availability (Testnet doesn't support multisig)

@jcronyn @syuan100
Multisig (#1)
e1964ee 
* Add Multisig Support

Co-authored-by: syuan100 <syuan100@gmail.com>
Co-authored-by: Joe <joe@cryptoballoon.net>
@vihu vihu requested a review from xandkar April 10, 2022 02:04
@syuan100 syuan100 mentioned this pull request Apr 10, 2022
Closed
1 task
tyler-whitman
tyler-whitman requested changes Apr 11, 2022
View reviewed changes
Copy link
Collaborator

@tyler-whitman tyler-whitman left a comment

Choose a reason for hiding this comment

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

This is looking great, have some review feedback and mostly lint errors to resolve.

packages/address/src/MultisigAddress.ts Outdated Show resolved Hide resolved
packages/address/src/MultisigAddress.ts Outdated Show resolved Hide resolved
packages/address/src/MultisigAddress.ts Outdated Show resolved Hide resolved
packages/address/src/MultisigAddress.ts Show resolved Hide resolved
packages/address/src/MultisigAddress.ts Outdated Show resolved Hide resolved
packages/crypto/src/MultisigSignature.ts Outdated
@@ -0,0 +1,121 @@
import Address, { KeyTypes, MultisigAddress } from '@helium/address'
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes will need to be brought over to the crypto-react-native package if we want to support this in the helium app. I am ok with doing that as a separate PR if wanted though.

packages/crypto/src/__tests__/MultisigSignature.spec.ts Outdated Show resolved Hide resolved
packages/crypto/src/utils.ts Outdated Show resolved Hide resolved
packages/transactions/src/PaymentV2.ts Outdated Show resolved Hide resolved
packages/transactions/src/PaymentV2.ts Outdated Show resolved Hide resolved
@jcronyn
Copy link
Contributor Author

jcronyn commented Apr 13, 2022 •
edited
Loading

This is looking great, have some review feedback and mostly lint errors to resolve.

All comments and lint issues addressed @tyler-whitman - Should be good to go :) Please let me know if there's anything else that needs updating.

Still need to end-to-end test this against testnet but otherwise should be good to go

EDIT: Cannot test end-to-end in testnet as multisig is not enabled...

@abhay
Copy link

abhay commented Apr 15, 2022

EDIT: Cannot test end-to-end in testnet as multisig is not enabled...

Multisig should work in testnet. what issues are you seeing?

@syuan100
Copy link
Contributor

@abhay miscommunication here. It works :) Just sent a payment to a multisig address here: https://testnet-explorer.helium.com/txns/hNMFq1t7CwwhRXOgL4uKb73i-_P8Oo0eVVtWXLhXYBU

@tyler-whitman
Copy link
Collaborator

@jcronyn I made a PR to fix the lint errors, with this running yarn lint will give no errors. Can you please take a look and merge that into this.

jcronyn#2

tyler-whitman
tyler-whitman approved these changes Apr 20, 2022
View reviewed changes
Copy link
Collaborator

@tyler-whitman tyler-whitman left a comment

Choose a reason for hiding this comment

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

Looks good to me, unit tests are passing and tests have been run on testnet. The last thing to do here would be to bring the changes over to crypto-react-native if we want MultiSig on mobile but for now I think it should be good to merge this and do that as another improvement.

Along with this I also pushed a fix to all the lint errors so now we can check lint more easily.

@allenan and @matthewcarlreetz mind taking a look before we merge this.

@jcronyn
Copy link
Contributor Author

jcronyn commented Apr 20, 2022

@tyler-whitman - Added a multi-signature example to the README.md. Feel free to review and let me know if you have any feedback. Thanks!

abhay
abhay reviewed Apr 20, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
allenan
allenan reviewed Apr 22, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
@allenan
Copy link
Member

allenan commented Apr 22, 2022

This is great work! my main feedback is that multisig feels a little shoehorned into the transactions library. It would be great to have a convenient way of calling it on an instance of a transaction, without requiring the instance to be mutated after the fact.

@jcronyn jcronyn requested a review from allenan April 23, 2022 00:24
@syuan100
Copy link
Contributor

syuan100 commented May 5, 2022

Hey y'all, wanted to check in on this for any outstanding items! Can we aggregate the last few asks if any and post it as a comment for visibility? Thanks y'all, excited to get this in 🍻

allenan
allenan approved these changes May 9, 2022
View reviewed changes
Copy link
Member

@allenan allenan left a comment

Choose a reason for hiding this comment

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

this looks good to me @jcronyn. I'll look into the conflicts

allenan and others added 2 commits May 9, 2022 10:30
@allenan @matthewcarlreetz
Fix merge conflicts, use async verify (#3)
99cc16f 
* Add message for burn verification.

* Verify burn txn.

* Create signature verification utility.

* Move @helium/crypto to dev dependencies.

* Remove old code.

* v4.1.0

Co-authored-by: Matt Reetz <matthewcarlreetz@gmail.com>
@jcronyn
Merge branch 'master' into master
88e6ccd
@allenan allenan merged commit 9f9ebe7 into helium:master May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhay abhay abhay left review comments

@syuan100 syuan100 syuan100 left review comments

@allenan allenan allenan approved these changes

@tyler-whitman tyler-whitman tyler-whitman approved these changes

@xandkar xandkar Awaiting requested review from xandkar

@matthewcarlreetz matthewcarlreetz Awaiting requested review from matthewcarlreetz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jcronyn @abhay @syuan100 @tyler-whitman @allenan @xandkar