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

Add support for empty namespaces #115

Merged
merged 7 commits into from
Sep 1, 2023

Conversation

cgrellard-ledger
Copy link
Contributor

@cgrellard-ledger cgrellard-ledger commented Aug 29, 2023

📝 Description

Some dapps needs to connect without any required namespace
Added the ability to connect without any accounts when there is no required namespace. In this case we hide the Accounts section in the Session Details page

We also now use the buildApprovedNamespaces util function from WalletConnect

❓ Context

See this thread for more context : https://ledger-slack-connect.slack.com/archives/C0402NGFF9U/p1692715205483189

📸 Demo

The accounts session isn't displayed when there is no accounts

Screenshot 2023-08-31 at 10 48 04

No required chain when trying to connect from https://se-sdk-dapp.vercel.app/

Screenshot 2023-08-31 at 10 47 49

🚀 Expectations to reach

Pull Requests must pass the CI and be internally validated in order to be merged.

Dapp to test : https://se-sdk-dapp.vercel.app/

Manifest to test

{
  "id": "ledger-wallet-connect",
  "name": "WalletConnect",
  "url": "https://wallet-connect-live-jf802e0em-ledgerhq.vercel.app/",
  "params": {},
  "homepageUrl": "https://walletconnect.com/",
  "icon": "https://forum.zeroqode.com/uploads/default/original/2X/e/e363c6521db27335d44c1134d230b8992792dde4.png",
  "platform": "all",
  "apiVersion": "^2.0.0",
  "manifestVersion": "1",
  "branch": "stable",
  "categories": ["bridge", "defi"],
  "currencies": [
    "ethereum",
    "polygon",
    "bsc",
    "optimism",
    "arbitrum",
    "avalanche_c_chain",
    "ethereum_goerli",
    "optimism_goerli"
  ],
  "content": {
    "shortDescription": {
      "en": "WalletConnect is an open source protocol for connecting decentralised applications to mobile wallets with QR code scanning or deep linking. V2 introduces new features, including the ability to connect to multiple dapps in parallel with multiple accounts. It's important to note that not all dapps currently support V2"
    },
    "description": {
      "en": "WalletConnect is an open source protocol for connecting decentralised applications to mobile wallets with QR code scanning or deep linking. V2 introduces new features, including the ability to connect to multiple dapps in parallel with multiple accounts. It's important to note that not all dapps currently support V2"
    }
  },
  "permissions": [
    "account.list",
    "account.request",
    "message.sign",
    "transaction.sign",
    "transaction.signAndBroadcast",
    "wallet.userId",
    "wallet.info"
  ],
  "domains": ["http://*", "https://*"]
}

Copy link
Member

@KVNLS KVNLS left a comment

Choose a reason for hiding this comment

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

I have not tested the code, but I trust that it works.
Some minor comments + Could you rebase to clean up your git history?

@@ -14,5 +14,5 @@ export const EIP155_SIGNING_METHODS = {
ETH_SIGN_TYPED_DATA_V3: "eth_signTypedData_v3",
ETH_SIGN_TYPED_DATA_V4: "eth_signTypedData_v4",
ETH_SEND_TRANSACTION: "eth_sendTransaction",
SWITCH_CHAIN: "wallet_switchEthereumChain",
// SWITCH_CHAIN: "wallet_switchEthereumChain",
Copy link
Member

Choose a reason for hiding this comment

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

🗑️ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment removed

{getDisplayName(chain)}
</Text>
</Box>
{Array.from(sessionAccounts).length > 0 ? (
Copy link
Member

Choose a reason for hiding this comment

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

Could you move getAccountsFromAddresses function outside of the component and return directly an array from sessionAccounts, please?
Today the useMemo dependencies do not reflect the reality as accounts is not used by the function getAccountsFromAddresses. By using it as a second parameter you can extract the function from the component (will not be redefined for each render) and the dependencies list of useMemo reflects what you use inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✅

Copy link
Member

@KVNLS KVNLS left a comment

Choose a reason for hiding this comment

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

Perfect, well done!

@cgrellard-ledger cgrellard-ledger merged commit a079111 into main Sep 1, 2023
2 checks passed
@mcayuelas-ledger mcayuelas-ledger deleted the feat/LIVE-9063-support-empty-namespaces branch September 5, 2023 14:16
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