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.

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: Centralise BigNumber imports #725

Merged
merged 11 commits into from
Sep 12, 2024
Merged

refactor: Centralise BigNumber imports #725

merged 11 commits into from
Sep 12, 2024

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Sep 4, 2024

Avoid importing BigNumber from ethers directly throughout the repository, and instead just re-export the standalone ethers BigNumber variant from src/utils/BigNumberUtils. This should make it easier to migrate away from ethers v5.

Avoid importing BigNumber from ethers directly throughout the
repository, and instead just re-export the standalone ethers BigNumber
variant from src/utils/BigNumberUtils. This should make it easier to
migrate away from ethers v5.
Copy link
Member

@nicholaspai nicholaspai 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 awesome work! So now we only have a single Bn import from ethers?

@pxrl
Copy link
Contributor Author

pxrl commented Sep 4, 2024

This is awesome work! So now we only have a single Bn import from ethers?

Yeah, hopefully this achieves that. It's a bit tricky to be 100% sure but I've eliminated as many cases of importing directly from ethers as I could identify. If we implement a BigInt -> BigNumber shim then we might be able to play with v6.

Copy link
Contributor

@gsteenkamp89 gsteenkamp89 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 really tedious to have implemented. 👏
Looks good though I'd make one improvement, since we don't know how long it's going to take to move over to viem, we can maybe add an eslint rule to restrict importing BigNumber from ethers?
https://eslint.org/docs/latest/rules/no-restricted-imports#paths

Copy link
Contributor

@dohaki dohaki 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! I think this shouldn't break anything on the FE AFAICT. Also really like @gsteenkamp89 suggestion

Copy link
Contributor

@gsteenkamp89 gsteenkamp89 left a comment

Choose a reason for hiding this comment

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

nice one! this will help for sure :)

@pxrl pxrl merged commit b55a6de into master Sep 12, 2024
4 checks passed
@pxrl pxrl deleted the pxrl/bn1 branch September 12, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants