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 sepolia testnet #61

Merged
merged 37 commits into from
Oct 3, 2023
Merged

feat: support sepolia testnet #61

merged 37 commits into from
Oct 3, 2023

Conversation

zixiang2018
Copy link
Contributor

@zixiang2018 zixiang2018 commented Sep 27, 2023

Context

What this PR does

@netlify
Copy link

netlify bot commented Sep 27, 2023

Deploy Preview for new-admin-opencerts ready!

Name Link
🔨 Latest commit 86f852b
🔍 Latest deploy log https://app.netlify.com/sites/new-admin-opencerts/deploys/651b948b6e77b10008086b19
😎 Deploy Preview https://deploy-preview-61--new-admin-opencerts.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@zixiang2018 zixiang2018 changed the title feat: bump ethers feat: support sepolia testnet Sep 28, 2023
src/app.tsx Outdated
Comment on lines 24 to 26
if (e instanceof EthereumProviderError) {
e.code === 4001 ? setMetamaskConnected(true) : console.debug(e);
}
Copy link
Member

Choose a reason for hiding this comment

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

Shall we also put an else to handle when e is not an instance of EthereumProviderError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Ill update it!

Actually, Ill also change the other error handling to log ? log(e.message) : console.error(e.message) instead of

log ? log(e.message) : null;
} else {
log ? log("Unable to issue certificate") : null;
where it does nothing when the log object is undefined

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be good as well to default to console.error(e.message)

When e is not an instance of Error, maybe we can do this:

log ? log("Unable to issue certificate", e) : console.error("Unable to issue certificate", e);
// or
log ? log("Unable to issue certificate", JSON.stringify(e)) : console.error("Unable to issue certificate", JSON.stringify(e));

Comment on lines 38 to 47
const addNetworkPromise = dappPage.evaluate(addNetwork);
try {
setTimeout(async () => {
await metamask.acceptAddNetwork(true);
}, 2000);
} catch (e) {
// ignore error
}

await addNetworkPromise;
Copy link
Member

Choose a reason for hiding this comment

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

Not too sure about this section. Should we be using await sleep(2000) instead of setTimeout()?

Furthermore, the last line can be:

await dappPage.evaluate(addNetwork);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow, if I add the await, it won't work... 🤔

Ill either have to separate the code (like the Dappeteer testcase) or just call dappPage.evaluate(addNetwork) before await metamask.acceptAddNetwork(true);

Anyway I removed the setTimeout from the code await metamask.acceptAddNetwork(true); as its no longer needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI about what the 2 code does,

dappPage.evaluate(addNetwork) opens a page to add the new network as per our configuration

metamask.acceptAddNetwork(true) will supposedly click on the button "Approve" to add the new network, and then do a switch of network to the new network (reference)

integration/utils.mjs Show resolved Hide resolved
@zixiang2018 zixiang2018 merged commit 10f07cf into master Oct 3, 2023
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.

2 participants