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

Use Ethers for AssetsContractController #845

Merged
merged 18 commits into from
Nov 14, 2022
Merged

Use Ethers for AssetsContractController #845

merged 18 commits into from
Nov 14, 2022

Conversation

FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented Jun 3, 2022 •
edited
Loading

Description
Swap out web3 for Ethers in the AssetsContractController, this was the final place the dependency was used.

Itemize the changes you have made into the categories below

  • CHANGED:
    • Swap out web3 for Ethers in the AssetsContractController

Checklist

  • Tests are included if applicable
  • Any added code is fully documented

@FrederikBolding FrederikBolding changed the title Basic Ethers implementation for AssetsContractController Use Ethers for AssetsContractController Nov 10, 2022
@FrederikBolding FrederikBolding marked this pull request as ready for review November 10, 2022 11:33
@FrederikBolding FrederikBolding requested a review from a team as a code owner November 10, 2022 11:33
@FrederikBolding
Copy link
Member Author

@Gudahtt Not sure if we want to do something to handle network changes or we should solve that in the extension? 🤔

mcmire
mcmire previously approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One suggestion but this looks good regardless. Really glad we can get rid of web3 🎉

src/assets/NftController.test.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
@socket-security
Copy link

socket-security bot commented Nov 12, 2022 •
edited
Loading

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Powered by socket.dev

Gudahtt
Gudahtt reviewed Nov 14, 2022
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks great! Just had a few questions

packages/assets-controllers/src/Standards/ERC20Standard.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/Standards/NftStandards/ERC721/ERC721Standard.ts Show resolved Hide resolved
packages/assets-controllers/src/Standards/ERC20Standard.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/Standards/NftStandards/ERC1155/ERC1155Standard.ts Show resolved Hide resolved
Gudahtt
Gudahtt previously approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

Gudahtt
Gudahtt previously approved these changes Nov 14, 2022
View reviewed changes
@FrederikBolding FrederikBolding merged commit 9b82201 into main Nov 14, 2022
@FrederikBolding FrederikBolding deleted the fb/use-ethers branch November 14, 2022 14:27
gantunesr pushed a commit that referenced this pull request Dec 8, 2022
@FrederikBolding @gantunesr
Use Ethers for AssetsContractController (#845)
ae1d980 
* Basic Ethers implementation for AssetsContractController

* Fix some tests

* Remove web3 from deps

* Fix lint

* Simplify a bit and fix more tests

* Fix more tests

* Fix tests by matching previous output types

* Fix issues after rebase

* Fix a bunch of tests and simplify

* Fix tests

* Fix lint

* Simplify

* Fix PR comments

* Update coverage

* Remove web3 types

* Remove use of then()

* Simplify decimals()

* Update coverage
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@FrederikBolding @MajorLift
Use Ethers for AssetsContractController (#845)
eaaa5a7 
* Basic Ethers implementation for AssetsContractController

* Fix some tests

* Remove web3 from deps

* Fix lint

* Simplify a bit and fix more tests

* Fix more tests

* Fix tests by matching previous output types

* Fix issues after rebase

* Fix a bunch of tests and simplify

* Fix tests

* Fix lint

* Simplify

* Fix PR comments

* Update coverage

* Remove web3 types

* Remove use of then()

* Simplify decimals()

* Update coverage
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@FrederikBolding @MajorLift
Use Ethers for AssetsContractController (#845)
b05bf5b 
* Basic Ethers implementation for AssetsContractController

* Fix some tests

* Remove web3 from deps

* Fix lint

* Simplify a bit and fix more tests

* Fix more tests

* Fix tests by matching previous output types

* Fix issues after rebase

* Fix a bunch of tests and simplify

* Fix tests

* Fix lint

* Simplify

* Fix PR comments

* Update coverage

* Remove web3 types

* Remove use of then()

* Simplify decimals()

* Update coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@mcmire mcmire mcmire left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@FrederikBolding @mcmire @Gudahtt