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/1295 - Update Namada Keychain integration #1298

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jurevans
Copy link
Collaborator

@jurevans jurevans commented Nov 21, 2024

resolves #1295
resolves #1270

  • Clean up Signer class (keep only signing-related functions, similar to Keplr interface)
  • Update namada.connect(), namada.disconnect() to accept optional chainId - 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 compatible
  • Namadillo should provide current chainId to extension on connect, is connected, and disconnect
  • Remove dependency on @namada/integrations - this may take a few iterations, but once Namadillo is completely updated, we can probably just remove this package
  • Fix Namadillo issues with detection (to trigger this behavior reliably, change something in Settings)
  • Update Faucet app to account for any API changes (will fix Docker build)
  • Approve Connection should also approve chainId for signing (now that we have the chainId - may need to adjust a bit so that pointing to a new indexer (if it has a different chainId) triggers an approval or Connect button
  • Remove deprecated viewing keys function in balance/atoms.ts
  • Fix tests
    • TODO: When we make 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 in Network

Notes

  • FYI, I had originally implemented a wallet manager for Namada Keychain, but I think it should only share a part of that interface since it wouldn't ever work well with the Keplr wallet manager, so I'm making a separate hook specifically for Namada Keychain, as our current integration is already separate

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:

  • From this branch, load Dry-Run settings and connect extension (you should be prompted to approve domain & enable signing for chainId:
    https://indexer.namada-dryrun.tududes.com
    https://rpc.namada-dryrun.tududes.com
    https://masp.namada-dryrun.tududes.com
  • Check Network in extension, this should be set to the current chain ID
  • Load a different indexer (Housefire) in Namadillo: https://indexer.knowable.run - you should be prompted to approve once the new chain ID is set
  • Check Network in extension, this should be set to the new chain ID. Also, you can check this in the console with: await namada.getChain()
  • Test sign Tx and sign-arbitrary
  • Test switching users and Disconnect
  • Run await namada.accounts() to see cleaned up data
  • You must now use const signer = namada.getSigner() instead of namada.signer() if you want to use the Signer API (same results as the namada.signTx() calls, only the usage is different as before).
  • Also, test for any cases where Download is displayed when the extension is installed - this should be fixed now!
  • In Chrome, launch a private tab to see the Download state (CMD+Shift+N)

@jurevans jurevans self-assigned this Nov 21, 2024
@jurevans jurevans force-pushed the feat/1295-refactor-keychain-integration branch 5 times, most recently from 3dfb491 to 924304f Compare November 22, 2024 10:45
@jurevans jurevans added this to the Namadillo Mainnet Readiness milestone Nov 22, 2024
@jurevans jurevans force-pushed the feat/1295-refactor-keychain-integration branch 9 times, most recently from 12e7889 to 6be9e40 Compare November 25, 2024 19:17
@jurevans jurevans changed the title WIP: feat/1295 - Update Namada Keychain integration feat/1295 - Update Namada Keychain integration Nov 26, 2024
@jurevans jurevans marked this pull request as ready for review November 26, 2024 10:58
@jurevans jurevans force-pushed the feat/1295-refactor-keychain-integration branch 2 times, most recently from b8490e6 to 3c48676 Compare November 26, 2024 17:22
Copy link
Contributor

@euharrison euharrison left a comment

Choose a reason for hiding this comment

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

Do you think we should render the chain id on the "Connected Sites" page?

And disconnect with the chain id as well?

Screenshot 2024-11-26 at 15 21 04

Screenshot 2024-11-26 at 15 21 50

@@ -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
Copy link
Contributor

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 🤔

Copy link
Collaborator Author

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!

Copy link
Collaborator Author

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> => {
Copy link
Contributor

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 :)

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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?

@jurevans
Copy link
Collaborator Author

Do you think we should render the chain id on the "Connected Sites" page?

And disconnect with the chain id as well?

@euharrison thanks for taking a look! Currently, chainId isn't tied to any domain. I have another PR that establishes a hierarchy of permissions here: #1142 - In that PR, chainId actually does something on those views, but in this PR, disconnect will simply clear the approved domain. I think for this PR chainId should not be used in those calls (but we can make it required so that it's more future-proof), then it will be available when permissions can be set by domain + by chain ID. Does that make sense?

@jurevans jurevans force-pushed the feat/1295-refactor-keychain-integration branch from 3c48676 to 87b739c Compare November 27, 2024 10:18
@jurevans jurevans force-pushed the feat/1295-refactor-keychain-integration branch 2 times, most recently from a31718a to ae29cd6 Compare November 27, 2024 17:40
@jurevans jurevans force-pushed the feat/1295-refactor-keychain-integration branch from ae29cd6 to de7291c Compare November 27, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants