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

[safesnap] Use public node to add network to metamask #2819

Closed
wants to merge 4 commits into from
Closed

[safesnap] Use public node to add network to metamask #2819

wants to merge 4 commits into from

Conversation

ChaituVR
Copy link
Member

@ChaituVR ChaituVR commented Jul 18, 2022 •
edited
Loading

@vercel
Copy link

vercel bot commented Jul 18, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
snapshot ✅ Ready (Inspect) Visit Preview Sep 2, 2022 at 7:56AM (UTC)

bonustrack
bonustrack requested changes Jul 18, 2022
View reviewed changes
Copy link
Member

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

Can you use getProvider instead?

@ChaituVR
Copy link
Member Author

It is actually used to add a new network to metamask, we don't actually need brovider/provider here, so replaced with public nodes

@ChaituVR ChaituVR changed the title Use brovider in safesnap [safesnap] Use public node to add network to metamask Jul 18, 2022
@bonustrack
Copy link
Member

Humm, who don't have mainnet on Metamask?

@ChaituVR
Copy link
Member Author

I overlooked this (twice), it is not just mainnet, should be able to add network same as safe,

also not good to add our RPC/brovider to user's metamask. maybe we can just remove this code block and show an error instad?

@ChaituVR
Copy link
Member Author

@bonustrack @mktcode Any thoughts on how to handle this?

@ChaituVR
Copy link
Member Author

Maybe we can use RPCs from here?

https://github.com/DefiLlama/chainlist/blob/main/constants/extraRpcs.json

@mktcode
Copy link
Contributor

mktcode commented Aug 2, 2022

I will work on Safesnap a lot in the coming days. Gonna think about it but right now my feeling says it's easiest to simply point to chainlist.org. The current solution only works for MetaMask anyway.

@ChaituVR ChaituVR mentioned this pull request Aug 11, 2022
Draft
@samuveth
Copy link
Contributor

@ChaituVR any update on this?

@ChaituVR ChaituVR added the invalid This doesn't seem right label Aug 27, 2022
@ChaituVR ChaituVR mentioned this pull request Aug 27, 2022
Closed
@ChaituVR
Copy link
Member Author

Closing for #3059

@ChaituVR ChaituVR closed this Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bonustrack bonustrack bonustrack requested changes

Assignees
No one assigned
Labels
invalid This doesn't seem right
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChaituVR @bonustrack @mktcode @samuveth