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

fix: internalize ethers functionality and remove dependency #2130

Merged
merged 47 commits into from May 10, 2024
Merged

fix: internalize ethers functionality and remove dependency #2130

merged 47 commits into from May 10, 2024

Conversation

maschad
Copy link
Member

@maschad maschad commented Apr 22, 2024 •
edited

Closes #2153

ethers is now removed from:

  • account
  • crypto

As a result there are more ethers functions being internalized:

  • ripemd160
  • pbkdf2
  • hmac
  • dataSlice
  • encodeBase58
  • decodeBase58
  • getBytes

This can be merged as a follow up to #2152

maschad and others added 25 commits April 11, 2024 18:04
@maschad
chore!: deprecate getNetwork method and remove ethers from account
49aabe5
@maschad
fix: update crypto functions, now builds
032ff1b
@maschad
lint: fixed linting issues + added changeset
aa63b05
@maschad
Merge branch 'master' into refactor/deprecate-get-network
4140987
@maschad
lint: update changeset title
1666de5
@maschad
test: add getBytes test
3beac58
@maschad
Merge branch 'master' into refactor/deprecate-get-network
7300377
@maschad
added more tests
93ee6ce
@maschad
test: fixed issue with base58encoding and added tests
2ceea7b
@maschad
Merge branch 'master' into refactor/deprecate-get-network
d4da9cc
@maschad
doc: update changeset
41b7cd6
@maschad
update changeset
311c46f
@maschad
remove unused dependencies
c1d4f09
@maschad
add test group to base58
4f5a300
@maschad
fix: update browser hmac
71de1a7
@arboleya
Merge branch 'master' into refactor/deprecate-get-network
416cd3e
@danielbate
Merge branch 'master' into refactor/deprecate-get-network
002a119
@maschad
test: fix issues with tests
d678f2f
@maschad
Merge branch 'master' into refactor/deprecate-get-network
48b09de
@maschad
test: update hmac tests
8f0a6d3
@maschad
test: update hmac test
54ceda4
@maschad
Merge branch 'master' into refactor/deprecate-get-network
f52e88a
@maschad
Merge branch 'master' into refactor/deprecate-get-network
5306d9b
@petertonysmith94 @Torres-ssf @maschad
docs: migrate wallet code snippets to doc-snippets (#1885)
933cd26 
* docs: moved wallet based doc tests, from fuel-guage to doc snippets

* chore: removed dev script

* docs: added wallet doc test to doc snippets

* chore: linting

* chore: removed redundant test file

* chore: update changeset

* chore: removed querying-the-chain (already in providers)

* chore: removed old doc-snippet

* chore: linting

* chore: fixing tests for wallet docs snippets

* chore: lint

* chore: removed doc snippet comments

---------

Co-authored-by: Sérgio Torres <[email protected]>
@maschad
build: upgrade pnpm version (#2125)
8f838ca
danielbate
danielbate reviewed May 2, 2024
View reviewed changes
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

Great work @maschad, another good step towards decoupling from ethers.

What was the reasoning behind implementing getBytes? As it looks exactly the same as our own arrayify?

packages/crypto/src/browser/pbkdf2.ts Outdated Show resolved Hide resolved
packages/crypto/test/hmac.node.test.ts Show resolved Hide resolved
packages/crypto/test/hmac.node.test.ts Show resolved Hide resolved
packages/crypto/test/pbkdf2.node.test.ts Show resolved Hide resolved
@maschad maschad requested a review from danielbate May 2, 2024 20:23
@maschad
Copy link
Member Author

maschad commented May 2, 2024

Great work @maschad, another good step towards decoupling from ethers.

What was the reasoning behind implementing getBytes? As it looks exactly the same as our own arrayify?

Thanks! There are some additional parameters included in the function signature for getBytes that I would need for some use cases I opted not to modify the arrayify to reduce the surface of these changes, but if that's acceptable I can include them in this PR.

@danielbate
Copy link
Contributor

IMO that is acceptable here, I'd rather have one point of entry for byte array conversion. I probably would have done the same when we initially internalised ethers if it didn't work with all the use cases.

danielbate
danielbate requested changes May 3, 2024
View reviewed changes
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

As discussed, amalgamate the functionality of getBytes and arrayify.

@maschad maschad requested a review from danielbate May 6, 2024 19:15
@maschad
Copy link
Member Author

maschad commented May 6, 2024

As discussed, amalgamate the functionality of getBytes and arrayify.

Thanks @danielbate I've updated in 69bf67a

danielbate
danielbate approved these changes May 10, 2024
View reviewed changes
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

Nice work 💪🏻

Jw, what ethers deps are left after this?

@maschad maschad enabled auto-merge (squash) May 10, 2024 14:40
@maschad
Copy link
Member Author

maschad commented May 10, 2024

Nice work 💪🏻

Jw, what ethers deps are left after this?

Thanks 😄

Looks like just doc-snippets and abi-coder now

@github-actions GitHub Actions
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.53%(-0.13%) 69.79%(+0%) 77.12%(-0.48%) 79.64%(-0.12%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 ✨ packages/crypto/src/browser/hmac.ts 0%
(+0%)		 0%
(+0%)		 0%
(+0%)		 0%
(+0%)
🔴 ✨ packages/crypto/src/browser/pbkdf2.ts 0%
(+0%)		 100%
(+100%)		 0%
(+0%)		 0%
(+0%)
🔴 ✨ packages/crypto/src/node/hmac.ts 46.66%
(+46.66%)		 0%
(+0%)		 0%
(+0%)		 46.66%
(+46.66%)
🔴 ✨ packages/crypto/src/node/pbkdf2.ts 46.66%
(+46.66%)		 0%
(+0%)		 0%
(+0%)		 46.66%
(+46.66%)
🔴 ✨ packages/crypto/src/shared/ripemd160.ts 69.23%
(+69.23%)		 0%
(+0%)		 50%
(+50%)		 71.42%
(+71.42%)
✅ ✨ packages/utils/src/utils/base58.ts 100%
(+100%)		 100%
(+100%)		 100%
(+100%)		 100%
(+100%)
✅ ✨ packages/utils/src/utils/dataSlice.ts 100%
(+100%)		 100%
(+100%)		 100%
(+100%)		 100%
(+100%)

@maschad maschad merged commit 316c757 into master May 10, 2024
19 checks passed
@maschad maschad deleted the refactor/internalize-ethers-deprecate-getnetwork branch May 10, 2024 19:43
@fuel-service-user fuel-service-user mentioned this pull request May 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielbate danielbate danielbate approved these changes

@petertonysmith94 petertonysmith94 petertonysmith94 approved these changes

@digorithm digorithm Awaiting requested review from digorithm digorithm is a code owner

@Torres-ssf Torres-ssf Awaiting requested review from Torres-ssf Torres-ssf is a code owner

@Dhaiwat10 Dhaiwat10 Awaiting requested review from Dhaiwat10 Dhaiwat10 is a code owner

@nedsalk nedsalk Awaiting requested review from nedsalk nedsalk is a code owner

@arboleya arboleya Awaiting requested review from arboleya arboleya is a code owner

Assignees

@maschad maschad

Labels
chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chore: remove ethers from account and crypto
8 participants
@maschad @fuel-service-user @danielbate @arboleya @petertonysmith94 @nedsalk @Torres-ssf @Dhaiwat10