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

feat: support for gnosis chain #76

Merged
merged 10 commits into from
Jul 5, 2023
Merged

feat: support for gnosis chain #76

merged 10 commits into from
Jul 5, 2023

Conversation

Venoox
Copy link
Contributor

@Venoox Venoox commented Jun 19, 2023

This approach determines chain id based on the token, DAI for mainnet and WXDAI for gnosis.

Maybe a better approach would be including chain id in the URL parameter when generating a claim url on the bot, which would be cleaner in my opinion.

Resolves #69

fixed explorer url
added gnosis rpc
export const chainRpc = {
"0x1": "https://rpc-pay.ubq.fi/v1/mainnet",
"0x5": "https://rpc-pay.ubq.fi/v1/goerli",
"0x64": "https://rpc.gnosischain.com",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we have a rpc for gnosis chain?

Copy link
Member

Choose a reason for hiding this comment

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

We do not have an internal one for Gnosis Chain. We currently do not have plans to make one yet, unless we can use a ready-made service like Cloudflare's (but they do not support Gnosis Chain last I checked.)

@0x4007
Copy link
Member

0x4007 commented Jun 19, 2023

Maybe a better approach would be including chain id in the URL parameter when generating a claim url on the bot, which would be cleaner in my opinion.

I agree I think that this is a good idea. I can add this to the bounty specification.

@0x4007 0x4007 mentioned this pull request Jun 19, 2023
4 tasks
@Venoox
Copy link
Contributor Author

Venoox commented Jun 19, 2023

Submitted a PR for chainID in permit url: ubiquity/ubiquibot#417

@Venoox Venoox marked this pull request as ready for review June 20, 2023 17:09
Comment on lines 3 to 7
export enum Chain {
Mainnet = "0x1",
Goerli = "0x5",
Gnosis = "0x64",
}
Copy link
Member

Choose a reason for hiding this comment

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

Seems more proper. Nice code quality.

Copy link
Member

@0x4007 0x4007 Jun 26, 2023

Choose a reason for hiding this comment

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

Please rename all references of "chain id" to "network id"

This is because I don't believe it is semantically correct to refer to rollups as "chains"

createToast("error", `Error: ${errorMessage}`);
};

const connectWallet = async (): Promise<JsonRpcSigner> => {
try {
const provider = new ethers.providers.Web3Provider(window.ethereum, "any");
const provider = new ethers.providers.Web3Provider((window as any).ethereum, "any");
Copy link
Member

@0x4007 0x4007 Jun 21, 2023

Choose a reason for hiding this comment

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

Don't use any try making or using a Window type.

@Venoox Venoox requested a review from 0x4007 June 22, 2023 21:40
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

The code looks high quality. I can't test from my phone but what I read through looks good.

@0xcodercrane I thought we have continuous deployment set up on all of our repos?

Strange that the last run was 2 months ago https://github.com/ubiquity/pay.ubq.fi/actions/workflows/continuous-deploy.yml of our continuous deploy CI: https://github.com/ubiquity/pay.ubq.fi/blob/main/.github/workflows/continuous-deploy.yml


export const chainRpc = {
[Chain.Mainnet]: "https://rpc-pay.ubq.fi/v1/mainnet",
[Chain.Goerli]: "https://rpc-pay.ubq.fi/v1/goerli",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is set up nor is it easy to do so we will need to fetch from chainlist.org possibly.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding types to this. I wrote this code like five years ago 😂

@0x4007 0x4007 changed the base branch from main to development June 24, 2023 09:19
@0x4007
Copy link
Member

0x4007 commented Jun 24, 2023

@0xcodercrane I thought we have continuous deployment set up on all of our repos?

I see that it is designed to not deploy when pull request opened to main so I just changed our default branch to development and changed this pull request base branch to development.

https://github.com/ubiquity/pay.ubq.fi/actions/runs/5363612713/jobs/9731306668 @0xcodercrane it doesn't work

Is it because it is conflicting with the Cloudflare Pages deployment I set up in my personal account?

image

@Venoox
Copy link
Contributor Author

Venoox commented Jun 24, 2023

@pavlovcik The workflow tries to create a project named pay.ubq.fi and that's invalid name because it has dots.

@0x4007
Copy link
Member

0x4007 commented Jun 24, 2023

Thanks for the clarification @Venoox! We'll need to substitute with valid characters, like hyphens.

@0x4007
Copy link
Member

0x4007 commented Jun 24, 2023

Can you push a commit to try it out?

https://github.com/ubiquity/pay.ubq.fi/actions/runs/5364165850/jobs/9732187832

Just realized Cloudflare generated an invalid URL or some issue with the SSL certificate because I can't load the linked URL https://41fa25b0.pay-ubq-fi-bxp.pages.dev/

@Venoox
Copy link
Contributor Author

Venoox commented Jun 24, 2023

@pavlovcik it seems that https://41fa25b0.pay-ubq-fi-bxp.pages.dev/out/styles/app.css is not loading the css but the html file, that usually occurs because the file does not exist so Cloudflare falls back to index.html.

I've looked at the workflow file again and it seems that the build command is not run, that's why /out directory doesn't exist.

@0x4007
Copy link
Member

0x4007 commented Jun 26, 2023

@Venoox I asked @rndquu to help with fixing the continuous deployment so that we can complete our review. Thanks for your patience.

@rndquu
Copy link
Member

rndquu commented Jun 27, 2023

@Venoox pls merge the latest development branch

@0x4007
Copy link
Member

0x4007 commented Jun 28, 2023

@rndquu following up

@rndquu
Copy link
Member

rndquu commented Jun 29, 2023

@Venoox pls merge the latest development branch

@rndquu
Copy link
Member

rndquu commented Jun 29, 2023

this PR can be checked here: https://f08f45ad.pay-ubq-fi-bxp.pages.dev/

I will add posting of deployment URLs today

@rndquu
Copy link
Member

rndquu commented Jun 29, 2023

@Venoox one more time pls merge the development branch

@github-actions
Copy link
Contributor

@ubiquity ubiquity deleted a comment from github-actions bot Jun 30, 2023
@Venoox
Copy link
Contributor Author

Venoox commented Jun 30, 2023

can we get this merged because I have #73 ready but it depends on this code

@0x4007
Copy link
Member

0x4007 commented Jul 3, 2023

The DOM seems to have gotten all messed up somehow, check the bottom of my screenshot. Upon mouseover the entire table to claim disappears as well.

I generated a test permit.

And I forgot to include the network ID in the url and the error message is unclear. Can you handle this error in the toaster? You can say something like the network ID needs to be passed in. Alternatively you can default to mainnet? I was quickly going through the source code and couldn't figure out what the query parameter's property name is.

image

The craziest part is that upon mouseover the entire page is an invalid link with embedded HTML which obviously isn't good.

image

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Please test before your next revisions with the encoded permit included in my test permit link to see the mouse-over DOM breaking issue which must be fixed.

@Venoox
Copy link
Contributor Author

Venoox commented Jul 3, 2023

@pavlovcik this is an issue with deployment because commit.txt does not exist and cloudflare pages returns the index.html which breaks everything

@rndquu
Copy link
Member

rndquu commented Jul 3, 2023

@pavlovcik this is an issue with deployment because commit.txt does not exist and cloudflare pages returns the index.html which breaks everything

I'll check it today

@rndquu
Copy link
Member

rndquu commented Jul 4, 2023

@pavlovcik this is an issue with deployment because commit.txt does not exist and cloudflare pages returns the index.html which breaks everything

pls merge the development branch, commit sha from the commit.txt file should appear

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023

@rndquu
Copy link
Member

rndquu commented Jul 4, 2023

@Venoox could you ensure that all of the errors are solved? If so then pls re-request a review

@Venoox Venoox requested a review from 0x4007 July 4, 2023 17:09
@Venoox
Copy link
Contributor Author

Venoox commented Jul 4, 2023

everything looks good now

@0x4007
Copy link
Member

0x4007 commented Jul 4, 2023

Just tested with the Ethereum mainnet permit, and connected to Gnosis Chain and it did what was expected.

Now I need to test the opposite. Gnosis Chain permit.

I need clarity on the query parameter to test the Gnosis Chain permit.

@Venoox
Copy link
Contributor Author

Venoox commented Jul 4, 2023

the query parameter is chainId so for gnosis chain it is: permit

@0x4007
Copy link
Member

0x4007 commented Jul 5, 2023

the query parameter is chainId so for gnosis chain it is: permit

This is looking fantastic. Thanks!

Please be sure to resolve all of the conversation points in the files changed view before we merge this, including renaming all instances of "chain" to "network" https://github.com/ubiquity/pay.ubq.fi/pull/76/files#r1242357293

Can you rename the query parameter to ?network=100 instead of using hexadecimal? Alternatively I suppose it wouldn't be too difficult to make the UI understand that network IDs prefixed with 0x are hexadecimal and should be parsed as base16.

I also want to give a small bonus when this is completed for 1. helping us debug the deployment issue and 2. the high quality code!

};

export const chainRpc = {
[Chain.Mainnet]: "https://rpc-pay.ubq.fi/v1/mainnet",
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be private. Perhaps this also can be fetched from chainlist etc. We can make a new bounty for this in order to close the current issue out faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by private?

Copy link
Member

@0x4007 0x4007 Jul 5, 2023

Choose a reason for hiding this comment

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

We should not share this RPC endpoint with the public because it can be abused (we don't have rate limits, API keys etc.)

This originally was set up strictly for our internal tooling because we would hammer the RPC with a LOT of network requests (public RPC endpoints would rate limit us.)

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

@Venoox
Copy link
Contributor Author

Venoox commented Jul 5, 2023

Now it's changed to network. Also query parameter network now supports hex and decimal

@0x4007 0x4007 merged commit ed640bd into ubiquity:development Jul 5, 2023
@0x4007
Copy link
Member

0x4007 commented Jul 5, 2023

I also want to give a small bonus when this is completed for 1. helping us debug the deployment issue and 2. the high quality code!

https://gnosisscan.io/tx/0xa7973600480751779489ca1f816e26f68d7f30653c2cdebcd3cbe8bb7f3bdc65

Thanks @Venoox

@Venoox
Copy link
Contributor Author

Venoox commented Jul 5, 2023

@pavlovcik thanks, I appreciate it!

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.

Gnosis Chain Support
3 participants