-
Notifications
You must be signed in to change notification settings - Fork 3
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: suggest chain #35
Conversation
8834eec
to
b78b12f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved contingent on a couple requests
// @ts-expect-error Check for Keplr | ||
const { keplr } = window; | ||
|
||
if (!keplr) { | ||
throw Error('Missing Keplr'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please drop the suppression.
// @ts-expect-error Check for Keplr | |
const { keplr } = window; | |
if (!keplr) { | |
throw Error('Missing Keplr'); | |
} | |
if (!('keplr' in window)) { | |
throw Error('Missing Keplr'); | |
} | |
const { keplr } = window; |
also consider a global declaration that adds keplr?: Keplr
to window so it's typed.
caption = `Agoric ${subdomain}`; | ||
} | ||
|
||
const walletUrlForStaking = `https://${url.hostname}.staking.agoric.app`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this template is more of a config. please group with other such config.
Sorry, I enabled automerge on this mistakenly thinking it was my other pull request. I'm going to follow up right now with suggested changes. |
fixes #32