-
Notifications
You must be signed in to change notification settings - Fork 107
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/1295 - Update Namada Keychain integration #1298
Conversation
3dfb491
to
924304f
Compare
12e7889
to
6be9e40
Compare
b8490e6
to
3c48676
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.
@@ -147,7 +147,8 @@ export class ConnectInterfaceResponseMsg extends Message<void> { | |||
|
|||
constructor( | |||
public readonly interfaceOrigin: string, | |||
public readonly allowConnection: boolean | |||
public readonly allowConnection: boolean, | |||
public readonly 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.
I'm wondering if this field should be mandatory 🤔
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 was hoping to make it optional while we transition to the new Namadillo, however, since the Signer class changed, it breaks the old Namadillo anyways :D I can easily make it required, will just need to update the approval tests!
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.
@euharrison So, to get this to work as a required param, we'll need to update @namada/integrations
(and the Faucet app) which I was hoping to avoid in this PR. There is the case of the Faucet where a chain ID isn't needed, as all it does is load accounts, so I'm wondering how we should handle that? I think for now, it shouldn't hurt to leave it as optional, and only set the Network setting if it is provided.
@@ -28,37 +27,17 @@ export type TokenBalance = AddressWithAsset & { | |||
dollar?: BigNumber; | |||
}; | |||
|
|||
const DEPRECATED_getViewingKey = async (): Promise<string | undefined> => { |
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.
friendly remember to merge from main before merging :)
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 explicitly took this out since that interface is no longer valid (no owner
exists in the extension). Are you saying it should be kept?
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 did rebase it to main
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.
ah, I see the new conflicts. I promise I won't merge with conflicts :D
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.
@jurevans unfortunately we still need to preserve this. Because we could have someone that do not updated the extension, but is already using the new Namadillo, then we still need to provide some support for them
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.
@euharrison I was thinking about this, and to keep that function, we'll also need to keep owner
in Extension, which means I need to revert some things in extension, right? As soon as we publish the Chrome extension, it should auto-upgrade on users machines, so we can assume that they'll be on the latest, and I plan to announce this to users on Discord who may be using the test build I provided.
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.
@euharrison just FYI, there are some other breaking changes if there is a mismatch between this Namadillo & extension, so if our plan is to support previous and current version, there will be quite a bit of handling needed, and I personally think it would be best to move everything to the latest, inform people to update their extension (it should be handled automatically for Chrome Store users), and ensure we get Namadillo published on Dry-Run & Housefire. Do you think that would be too problematic?
@euharrison thanks for taking a look! Currently, |
3c48676
to
87b739c
Compare
ae29cd6
to
de7291c
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.
LGMT ✅
de7291c
to
646d341
Compare
acfd9e2
to
be8ecd6
Compare
be8ecd6
to
18babd0
Compare
resolves #1295
resolves #1270
Signer
class (keep only signing-related functions, similar to Keplr interface)namada.connect()
,namada.disconnect()
to accept optionalchainId
- this will be used to set approved signing chain, but may not be added in this PR (depending on time) but it will be good to have to be backward compatiblechainId
to extension on connect, is connected, and disconnect@namada/integrations
- this may take a few iterations, but once Namadillo is completely updated, we can probably just remove this packagechainId
for signing (now that we have thechainId
- may need to adjust a bit so that pointing to a new indexer (if it has a differentchainId
) triggers an approval orConnect
buttonbalance/atoms.ts
chainId
required for approvals, update tests to reflect this. For now, it should work the old way as well, only it won't update the value inNetwork
Notes
TESTING
NOTE In general, we just want to make sure any extension functionality works as expected, and if the extension is installed, you should never see the
Download
button. The following is how I tested this:Network
in extension, this should be set to the current chain IDhttps://indexer.knowable.run
- you should be prompted to approve once the new chain ID is setNetwork
in extension, this should be set to the new chain ID. Also, you can check this in the console with:await namada.getChain()
Disconnect
await namada.accounts()
to see cleaned up dataconst signer = namada.getSigner()
instead ofnamada.signer()
if you want to use the Signer API (same results as thenamada.signTx()
calls, only the usage is different as before).Download
is displayed when the extension is installed - this should be fixed now!Download
state (CMD+Shift+N
)