-
Notifications
You must be signed in to change notification settings - Fork 128
feat/552 - Updates to integrations, better support chain ID updates #554
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
Conversation
1342ca5 to
f16ef44
Compare
dd10c48 to
98208aa
Compare
98208aa to
7d0579d
Compare
7d0579d to
b96ffae
Compare
f496549 to
c662bb9
Compare
d6c6528 to
f9aefc4
Compare
f9aefc4 to
3649afc
Compare
c288537 to
c0465ab
Compare
c0465ab to
1af132b
Compare
1af132b to
50b1882
Compare
50b1882 to
dce745e
Compare
dce745e to
95832d9
Compare
167cc9a to
dcb6875
Compare
dcb6875 to
03b5b3a
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.
Code looks good to me! I just left some questions where I'm not familiar with the code.
It didn't work for me at first, but I just needed to clear the cache to get rid of the old chain config.
| DerivedAccount, | ||
| "address" | "alias" | "type" | "publicKey" | ||
| > & { | ||
| chainId: string; |
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.
Do we still need to have chainId here now that chainKey is added?
| NAMADA_INTERFACE_NAMADA_BECH32_PREFIX: bech32Prefix = DEFAULT_BECH32_PREFIX, | ||
| } = process.env; | ||
|
|
||
| const namada: Chain = { |
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.
Not a problem with this PR, but I think chainId and rpc here are going to be wrong if they change in the extension. Although actually, is the RPC setting in the extension meant to apply to the interface too or just the extension?
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.
yep, that's expected! These are just initial values. One of the things I needed to address is that the Network settings in extension are always read as the source of truth. RPC applies to both, since Query & SDK use it in extension, and the interface also uses Query, so the chain RPC setting in the interface must match the extension always (the query to load any new chain settings from extension is important)
| { state: RootState } | ||
| >(`${PROPOSALS_ACTIONS_BASE}/${ProposalsActions.FetchProposals}`, async () => { | ||
| const { rpc } = chains[defaultChainId]; | ||
| const { rpc } = chains.namada; |
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.
In some cases, we are getting the Namada chain config from Redux state, and in others using chains.namada. What is the difference?
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.
oh, this is wrong! That should be pulled from Redux state. I can add it here, or in a follow up PR. I mainly focused on getting everything confirmed to work at least for transfers (to confirm extension is correct), but yeah, this needs to use Redux state here.
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.
I just pushed up a fix for this, and also the Staking actions were referring to default config instead of the Redux state
3987d1b to
4f5244a
Compare
resolves #552
0.2.1useIntegration) to index instances by aChainKeywhich is"namada" | "cosmos" | "ethereum"for now, instead of chain Id, as the chain ID can be changed on the Namada side in the extensionChainKeydefaultChainIdexport (and related ones from Cosmos, etc.)chain IDin Extension does not break association with integration nowchainIdfor Reveal PK (ConfirmLedgerTx)rpc&chainIdfrom the extension settings (which user may override)Testing
If you have balances on the current public testnet, then testing is pretty easy. This is the default value in the chain config for Namada, so the extension & interface can be built without any
.envvariables - if they are defined, they only affect the initial state of the chain config in the extension, and a user can override those with theNetworkformNetworkform in the interface, and update the Chain ID & URLAlso, we can test integrations on the IBC page - when switching Network around, and without refreshing the interface, you should see that Keplr & Namada integrations always work