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

feat(sdk): migrate to ethers v6 #2374

Open
wants to merge 122 commits into
base: feat/major-poc
Choose a base branch
from
Open

feat(sdk): migrate to ethers v6 #2374

wants to merge 122 commits into from

Conversation

zaidarain1
Copy link
Contributor

No description provided.

@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Nov 13, 2024 •
edited
Loading

View your CI Pipeline Execution ↗ for commit 7916bfc.

Command Status Duration Result
nx affected -t build,lint,test ✅ Succeeded 2m 7s View ↗
nx run-many -p @imtbl/sdk,@imtbl/checkout-widge... ✅ Succeeded 1s View ↗

☁️ Nx Cloud last updated this comment at 2025-02-12 00:54:21 UTC

@zaidarain1 zaidarain1 force-pushed the feat/ethers-v6-migration branch 3 times, most recently from d00fbd7 to 1c0a592 Compare November 26, 2024 23:56
@zaidarain1 zaidarain1 force-pushed the feat/ethers-v6-migration branch from a6c281e to bdafd2d Compare November 27, 2024 06:17
@zaidarain1 zaidarain1 marked this pull request as ready for review November 28, 2024 01:57
@zaidarain1 zaidarain1 requested review from a team as code owners November 28, 2024 01:57
@zaidarain1 zaidarain1 requested review from shineli1984 and a team as code owners November 29, 2024 06:13
@zaidarain1 zaidarain1 force-pushed the feat/ethers-v6-migration branch 2 times, most recently from cb7c27d to c0d1cfb Compare December 5, 2024 05:04
@zaidarain1 zaidarain1 force-pushed the feat/ethers-v6-migration branch 4 times, most recently from b094201 to 2d08363 Compare December 6, 2024 00:03
Sam-Jeston
Sam-Jeston reviewed Dec 8, 2024
View reviewed changes
Copy link
Contributor

@Sam-Jeston Sam-Jeston left a comment

Choose a reason for hiding this comment

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

The orderbook updates make sense to me. One nit around remaining v5 naming that doesnt apply anymore

packages/orderbook/src/seaport/transaction.ts Outdated Show resolved Hide resolved
@zaidarain1 zaidarain1 force-pushed the feat/ethers-v6-migration branch 2 times, most recently from 140adfc to d321490 Compare December 9, 2024 03:15
@zaidarain1 zaidarain1 changed the title migrate to ethers v6 feat(sdk): migrate to ethers v6 Dec 12, 2024
@zaidarain1 zaidarain1 changed the title feat(sdk): migrate to ethers v6 migrate to ethers v6 Dec 12, 2024
@zaidarain1 zaidarain1 force-pushed the feat/ethers-v6-migration branch from 09f6e24 to 36f069a Compare January 31, 2025 05:22
@keithbro-imx keithbro-imx self-requested a review February 3, 2025 00:35
@zaidarain1 zaidarain1 force-pushed the feat/ethers-v6-migration branch from 256faa6 to 313ed85 Compare February 3, 2025 02:12
haydenfowler
haydenfowler reviewed Feb 10, 2025
View reviewed changes
packages/internal/generated-clients/src/imx-openapi.json
},
"host": "api.sandbox.x.immutable.com",
"host": "api.sandbox.immutable.com",
Copy link
Contributor

Choose a reason for hiding this comment

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

The generated-clients changes that relate to an updated OpenAPI spec can probably be pulled out into a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of those changes are related to this PR, as the main change was the updated chainID typing in the EIP712Domain type change, which was a required update to support the ethers v6 migration. If preferred, I can undo all the other unrelated changes but keep that one set of changes. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@zaidarain1 I believe @shirren has created a separate PR for this here

haydenfowler
haydenfowler reviewed Feb 10, 2025
View reviewed changes
packages/passport/sdk/src/zkEvm/zkEvmProvider.ts Outdated
this.#rpcProvider = new StaticJsonRpcProvider({
url: this.#config.zkEvmRpcUrl,
fetchOptions: { referrer: config.jsonRpcReferrer },
this.#rpcProvider = new JsonRpcProvider(this.#config.zkEvmRpcUrl, undefined, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we'll break the Game SDK(s) by no longer setting the referrer to config.jsonRpcReferrer 🤔 Have we run this change by the Game SDK team?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ethers v6 doesn't set "client" as the referrer anymore, and referrer isn't able to be set for the provider either, so this shouldn't be a problem with "client" being set by default.

This was something that I was planning on testing with and bringing up to GameSDK once the SDK was finalised and approved from the @imtbl/sdk npm package side though before release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice! With that in mind then, we can remove the jsonRpcReferrer arg from the config and Passport constructor , and simplify this logic

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

@haydenfowler haydenfowler haydenfowler left review comments

@Sam-Jeston Sam-Jeston Sam-Jeston left review comments

@luads luads luads approved these changes

@Benjimmutable Benjimmutable Benjimmutable approved these changes

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

@keithbro-imx keithbro-imx Awaiting requested review from keithbro-imx

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zaidarain1 @luads @haydenfowler @Sam-Jeston @Benjimmutable @glomotion