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(builder): Replace viem getUrl calls to registry with axios #1351

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix(builder): Replace viem getUrl calls to registry with axios #1351

wants to merge 9 commits into from

Conversation

FuzzB0t
Copy link
Contributor

@FuzzB0t FuzzB0t commented Sep 3, 2024

Replaces getUrl and getMetaUrl to use axios to make rpc calls to the registry

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Sep 3, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: ac73ed9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@FuzzB0t FuzzB0t requested a review from a team September 3, 2024 22:06
@FuzzB0t FuzzB0t changed the title fix(builder): Replace get calls to registry with axios fix(builder): Replace getUrl calls to registry with axios Sep 4, 2024
@FuzzB0t FuzzB0t changed the title fix(builder): Replace getUrl calls to registry with axios fix(builder): Replace viem getUrl calls to registry with axios Sep 4, 2024
@saeta-eth
Copy link
Contributor

What is the motivation for this PR? I am out of the loop and it seems confusing

@FuzzB0t
Copy link
Contributor Author

FuzzB0t commented Sep 6, 2024

What is the motivation for this PR? I am out of the loop and it seems confusing

@saeta-eth
We we're seeing a lot of issues with the registry get calls and RPC's so db mentioned we could optimize the calls by using axios to make rpc requests instead of viem. There may be other reasons for this PR but that was the main one AFAIK.

mjlescano
mjlescano reviewed Sep 10, 2024
View reviewed changes
packages/builder/src/registry.ts
@@ -480,7 +480,36 @@ export class OnChainRegistry extends CannonRegistry {
],
});

return (url as string) || null;
const response = await axios.post(
Copy link
Member

Choose a reason for hiding this comment

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

This call could be moved to a helper like contractFunctionCall so this method does not get super long.

mjlescano
mjlescano reviewed Sep 10, 2024
View reviewed changes
packages/builder/src/registry.ts
@@ -504,6 +533,35 @@ export class OnChainRegistry extends CannonRegistry {
],
});

const response = await axios.post(
Copy link
Member

Choose a reason for hiding this comment

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

Here it can use the helper mentioned before.

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

@mjlescano mjlescano mjlescano left review comments

@dbeal-eth dbeal-eth Awaiting requested review from dbeal-eth

At least 1 approving review is required to merge this pull request.

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
@FuzzB0t @saeta-eth @mjlescano