-
Notifications
You must be signed in to change notification settings - Fork 87
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
refactor: copy account module from @celo/cryptographic-utils@3.2.0 #5861
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5861 +/- ##
==========================================
+ Coverage 87.61% 87.63% +0.01%
==========================================
Files 738 739 +1
Lines 31650 31861 +211
Branches 5637 5710 +73
==========================================
+ Hits 27730 27921 +191
- Misses 3697 3896 +199
+ Partials 223 44 -179
... and 72 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
97273d0
to
d34bdb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming that the contents of account.ts
and account.test.ts
are basically copied verbatim (which, from a brief comparison to the originals, it looks like they are).
@@ -0,0 +1,495 @@ | |||
// Initially copied from https://github.com/celo-org/celo-monorepo/blob/sdks-3.2.0/packages/sdk/cryptographic-utils/src/account.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is any part of this file meaningfully modified from the original? What of this ought to be reviewed, if any?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
This is the first step to remove
@celo/cryptographic-utils
.It copies the accounts module from it.
I've copied the exact same version used from the celo SDK:
3.2.0
.And listed the exact same NPM packages it was using internally.
This is to ensure it behaves the same.
We'll upgrade/remove these in a follow up PR.
I've slightly modified it to directly include some sub modules, which we didn't need to share.
@celo/base/lib/account
(some type definitions)@celo/base/lib/string
(only 1 function used)Test plan
Related issues
Backwards compatibility
Yes
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: