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

fix: Paymaster Proxy URL #513

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

cpcramer
Copy link
Contributor

@cpcramer cpcramer commented Jun 7, 2024

What changed? Why?

We are checking if process.env.NEXT_PUBLIC_VERCEL_ENV is undefined which means that we are on local. Therefore, we will hit our Paymaster directly.

When deployed we are using the System Environment Variable NEXT_PUBLIC_VERCEL_PROJECT_PRODUCTION_URL.
Notes to reviewers

How has it been tested?
Tested locally and in Vercel Dev.

@cpcramer cpcramer changed the title Paul/test onchain kit paymaster functionality 🚧 Test onchain kit paymaster functionality Jun 7, 2024
@cpcramer cpcramer force-pushed the paul/test--onchain-kit-paymaster-functionality branch 2 times, most recently from f1fe759 to e5bcfe6 Compare June 10, 2024 15:14
@cpcramer cpcramer changed the title 🚧 Test onchain kit paymaster functionality 🚧fix: Paymaster Proxy URL Jun 10, 2024
@cpcramer cpcramer changed the title 🚧fix: Paymaster Proxy URL fix: Paymaster Proxy URL Jun 10, 2024
@cpcramer cpcramer requested a review from 0xAlec June 10, 2024 17:10
@cpcramer cpcramer requested a review from Zizzamia June 11, 2024 15:37
const defaultUrl = isLocalEnv ? paymasterURL : `${document.location.origin}/api/paymaster-proxy`;
// Target the Paymaster directly without a proxy if running on localhost.
// Use the Paymaster Proxy when deployed.
const isLocalEnv = process.env.NEXT_PUBLIC_VERCEL_ENV === undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with isLocal? should we fix that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isLocal wasn't working for me. It was always returning that the application was running locally, even when deployed.

Copy link
Contributor Author

@cpcramer cpcramer Jun 11, 2024

Choose a reason for hiding this comment

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

I think that we can just take advantage of the Vercel System Environment Variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually looked like the issue was with getCurrentEnvironment - a function that isLocal relies on.

Copy link
Contributor

Choose a reason for hiding this comment

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

My advice here is, to make sure isLocal() works well. And just use that everywhere.

And avoid doing side solution.

In a few words, improve what we have, and avoid create N+1 solution for the same problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I updated getCurrentEnvironment to have the NEXT_PUBLIC prefix and it now works. I've tested in local and dev!

@Zizzamia Zizzamia merged commit 462e848 into main Jun 11, 2024
9 checks passed
@Zizzamia Zizzamia deleted the paul/test--onchain-kit-paymaster-functionality branch June 11, 2024 21:46
cpcramer added a commit that referenced this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants