Skip to content

feat: updating setup script to use orbit sdk - #72

Open
GreatSoshiant wants to merge 14 commits into
mainfrom
orbit-sdk
Open

feat: updating setup script to use orbit sdk #72
GreatSoshiant wants to merge 14 commits into
mainfrom
orbit-sdk

Conversation

@GreatSoshiant

Copy link
Copy Markdown
Member

Closes: INT-136

Comment threadscripts/l3Configuration.ts
Comment threadscripts/l3Configuration.ts Outdated
Comment threadscripts/l3Configuration.ts Outdated
Comment threadscripts/l3Configuration.ts Outdated
Comment threadscripts/l3Configuration.ts Outdated
Comment threadscripts/transferOwnership.ts Outdated
Comment on lines +50 to +51
l3Provider: ethers.providers.JsonRpcProvider,
childChainRpc: string

@TucksonDevTucksonDevApr 10, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think l3Provider includes the RPC URL in the object, can you verify? In that case we wouldn't need the last parameter.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No JsonRpcProvider doesn't have any method to get rpc url which is weird haha
https://docs.ethers.org/v5/api/providers/jsonrpc-provider/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can get it from l3Provider.connection.url. Just did a quick test, but verify on your end 🙏

@TucksonDev

Copy link
Copy Markdown
Contributor

The script works and files are much cleaner now, thanks @GreatSoshiant !
I've added a few comments to improve clarity further specially with variable naming, so it's easier to work on the script in the future.

@TucksonDevTucksonDev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks very much @GreatSoshiant !
There's one minor comment pending, but this LGTM 🚀

Sign up for freeto 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.

2 participants

@GreatSoshiant@TucksonDev