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/view claim #251

Merged
merged 3 commits into from
Jun 17, 2024
Merged

Fix/view claim #251

merged 3 commits into from
Jun 17, 2024

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Jun 13, 2024

Resolves #250

@Keyrxng Keyrxng requested a review from rndquu June 13, 2024 16:38
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Jun 13, 2024

Copy link
Contributor

github-actions bot commented Jun 13, 2024

@@ -100,12 +100,17 @@ async function transferFromPermit(permit2Contract: Contract, app: AppState) {
async function waitForTransaction(tx: TransactionResponse) {
try {
const receipt = await tx.wait();
viewClaimButton.onclick = () => {
window.open(`https://${tx.chainId === 1 ? "etherscan" : "gnosisscan"}.io/tx/${receipt.transactionHash}`, "_blank");
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a bad approach. Why would you only support one or the other if we are supposed to support every EVM chain with a permit2 deployment?

Copy link
Contributor Author

@Keyrxng Keyrxng Jun 15, 2024

Choose a reason for hiding this comment

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

Good point, the most robust solution I could come up with is to use Blockscan. Check the link, it's the Permit2 address and on the left hand side there is a dropdown of the chains that are supported (26).

Blockscan TX

The reason why I've used this approach over building our own dataset of chain explorers as that's what we'd need to do is because they don't share a uniform URL schema so we'd need to manually collect chains that either we'd allow support for or just a more robust collection than what Blockscan currently has.

Copy link
Member

@0x4007 0x4007 Jun 15, 2024

Choose a reason for hiding this comment

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

Manually collect seems fine. I recall like eight networks some of which are test networks.

I can't find the table of networks from my phone but check the uniswap docs.

Copy link
Contributor Author

Copy link
Contributor Author

@Keyrxng Keyrxng Jun 15, 2024

Choose a reason for hiding this comment

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

Best to open an issue in rpc-handler to update the network explorers and then use that as a single source across projects then


that is a solid list for mainnets, do you have any other specific chains in mind that are not included here?

I can't find that table you mentioned either but I'll be able to track down the explorers anyway, I'll aim to cover at least mainnet and it's testnet and if it has multiple testnets like ETH, I'll grab the most relevant ones.

Copy link
Member

@0x4007 0x4007 Jun 17, 2024

Choose a reason for hiding this comment

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

I just quickly looked up where it has activity... Except I'm not 100% sure its deployed on every one of these chains.

https://blockscan.com/address/0x000000000022d473030f116ddee9f6b43ac78ba3#portfolios

blockscan com_address_0x000000000022d473030f116ddee9f6b43ac78ba3

We should manually test that it works on the main chains, such as: Ethereum mainnet, Optimism, Arbitrum, Polygon, Gnosis, Base

@0x4007 0x4007 merged commit aa003e2 into ubiquity:development Jun 17, 2024
4 checks passed
Sign up for free to 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.

"View claim" button doesn't work
2 participants