Skip to content

feat: Multisig Support #156

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
Merged
merged 9 commits into from
Apr 1, 2022
Merged

feat: Multisig Support #156

merged 9 commits into from
Apr 1, 2022

Conversation

r4mmer
Copy link
Member

@r4mmer r4mmer commented Mar 10, 2022

Design

Acceptance Criteria

Create new endpoints as defined on the design:

  • /multisig-pubkey
  • /wallet/decode
  • /wallet/signature
  • /wallet/partial-tx
  • /wallet/tx-assemble-push

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@r4mmer r4mmer added the enhancement New feature or request label Mar 10, 2022
@r4mmer r4mmer self-assigned this Mar 10, 2022
@codecov
Copy link

codecov bot commented Mar 11, 2022 •
edited
Loading

Codecov Report

Merging #156 (33ab595) into dev (add9c37) will increase coverage by 2.38%.
The diff coverage is 94.21%.

@@            Coverage Diff             @@
##              dev     #156      +/-   ##
==========================================
+ Coverage   83.93%   86.31%   +2.38%     
==========================================
  Files           7        8       +1     
  Lines         473      592     +119     
  Branches      104      131      +27     
==========================================
+ Hits          397      511     +114     
- Misses         69       74       +5     
  Partials        7        7
Impacted Files Coverage Δ
src/api-docs.js 100.00% <ø> (ø)
src/constants.js 0.00% <0.00%> (ø)
src/index.js 87.89% <95.00%> (+2.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update add9c37...33ab595. Read the comment docs.

tuliomir
tuliomir reviewed Mar 21, 2022
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Mar 22, 2022

This pull request introduces 1 alert when merging 6ccdc37 into 02ed7c9 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 23, 2022

This pull request introduces 1 alert when merging d9c05bf into 02ed7c9 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 23, 2022

This pull request introduces 1 alert when merging b804467 into 02ed7c9 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@r4mmer r4mmer marked this pull request as ready for review March 24, 2022 13:36
@r4mmer r4mmer requested a review from pedroferreira1 as a code owner March 24, 2022 13:36
@lgtm-com
Copy link

lgtm-com bot commented Mar 24, 2022

This pull request introduces 6 alerts when merging 1975357 into 02ed7c9 - view on LGTM.com

new alerts:

  • 5 for Useless conditional
  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 24, 2022

This pull request introduces 5 alerts when merging 39e97db into 02ed7c9 - view on LGTM.com

new alerts:

  • 5 for Useless conditional

@lgtm-com
Copy link

lgtm-com bot commented Mar 24, 2022

This pull request introduces 5 alerts when merging 9a843cc into 02ed7c9 - view on LGTM.com

new alerts:

  • 5 for Useless conditional

@r4mmer r4mmer requested a review from tuliomir March 25, 2022 20:48
tuliomir
tuliomir requested changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@tuliomir tuliomir left a comment

Choose a reason for hiding this comment

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

Only found some minor fixes, a few doubts and JSDocs improvements.

__tests__/start.test.js
expect(response1.status).toBe(200);
expect(response1.body.success).toBeFalsy();

global.config.multisig = TestUtils.multisigData;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we must address this topic here, but there is a very small chance that, by playing with global variables, other tests within this same start.test.js suite may have unintended side-effects.

As our tests need to be independent of each other inside the same suite, a possible solution could be having all these assertions that depend on the global variable within the same it clause. That way, they would be running deterministically.
Not very readable, but it avoids this small risk.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should improve the way we mock the configuration (using jest mock instead of global), this would solve the problem and wouldn't change the readability of the tests.
A change like this would be better on it's own PR, and since using the global works (at least for now) we can use it like this until we change the configuration mock.

pedroferreira1
pedroferreira1 requested changes Mar 30, 2022
View reviewed changes
tuliomir
tuliomir approved these changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@tuliomir tuliomir left a comment

Choose a reason for hiding this comment

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

By my side, all approved.
It would be good to define if the CodeCov notifications should be addressed on this PR.

pedroferreira1
pedroferreira1 requested changes Mar 31, 2022
View reviewed changes
@r4mmer r4mmer requested a review from pedroferreira1 March 31, 2022 16:01
@r4mmer r4mmer merged commit 5b360e2 into dev Apr 1, 2022
@r4mmer r4mmer deleted the feat/multisig branch April 1, 2022 14:26
This was referenced Apr 19, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuliomir tuliomir tuliomir approved these changes

@pedroferreira1 pedroferreira1 pedroferreira1 approved these changes

Assignees

@r4mmer r4mmer

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@r4mmer @tuliomir @pedroferreira1