Feat/add GridPlus Lattice1 Hardware Wallet #1346
Conversation
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
|
@brunobar79 the PR feedback improvements are implemented now. |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
Here's the packed extension for this build: |
There was a problem hiding this comment.
did a quick lil onboarding test using mock data / IS_TESTING. Only issues I see:
- Wallets & Keys page uses the generic Ledger logo instead of the Grid Lattice + logo
- The first two pages of the flow don’t seem to match our other designs. mainly font size / spacing. this is just me nit picking tho. may need designer input.
There was a problem hiding this comment.
Finally got to test this with a physical GridPlus Lattice today:
- Import flow worked well
- Eth mainnet swaps, sends + dapp interactions are the only thing that seemed to work for me. No L2s or sidechains. When attempting, the GridPlus screen would flash, then the browser extension would say ‘make sure your GridPlus is unlocked’
- Importing a duplicate wallet just caused the gridPlus import flow to get stuck on this screen:
- Also when you import a wallet with the GridPlus, then import a sub wallet under the same mnemonic, they appear as separate wallet groups in our Wallets & Keys:
- Additionally, it would be nice if the flow was keyboard navigable. I have suggestions on how to make the first page of the flow work with our keyboard nav. You can add
tabIndex={0}on most things to make it work, or in some cases, you can also use ourLenscomponent.
…-extension into feat/add-gridplus-lattice1
|
@BrodyHughes @brunobar79 the new feedback is addressed now. Pushed the improvements. |
|
Do you have a more square sized asset for this screen so it matches the designs of the others? Also still seeing these issues:
|
|
Hi @BrodyHughes @brunobar79, I refactored the tx signing part and L2 transactions should now be working fine. Also the logo in Wallets & Keys is updated to match the rest. |
| const nonExistingAddresses = fetchedAddresses | ||
| .map((address) => getAddress(address)) | ||
| .filter( | ||
| (address) => | ||
| !sortedAccounts.map((account) => account.address).includes(address), | ||
| ); |
There was a problem hiding this comment.
is the idea here to not let the user select accounts that he already have in the extension?
lets say he had the pk in the extension AND in the grid+ lattice?
this should prevent him from importing that account from the gridplus?
There was a problem hiding this comment.
Yes, precisely. It resulted in a thrown error when importing the same address another time.
| deviceId: formData.deviceId, | ||
| password: formData.password, | ||
| name: appName, | ||
| getStoredClient: () => useGridPlusClientStore.getState().client, |
There was a problem hiding this comment.
why do we have a useGridPlusClientStore and a getStoredGridPlusClient?
There was a problem hiding this comment.
Hi @greg-schrammel! Our SDK currently only supports synchronous storages (it's Web Storage API compliant), so in order to upgrade from it, to async Chrome Storage API, we had to utilize a simple Zustand store to make it synchronous.
| (account) => account.address, | ||
| ); | ||
| } else { | ||
| fetchedAddresses = (await fetchAddresses()) as Address[]; |
There was a problem hiding this comment.
does this fetchAddresses get the client from some global? how does it know? couldn't find it in the sdk docs
There was a problem hiding this comment.
fetchAddresses calls our API. Now, if user permitted Rainbow, it will return n derived addresses of user's wallet. The detailed implementation:
https://github.com/GridPlus/gridplus-sdk/blob/32b5389a768b2efd989a8af892a2b14e837110a8/src/api/addresses.ts#L18
| event.preventDefault(); | ||
| const accountsToImport = formData.selectedAddresses.map((address, i) => ({ | ||
| address, | ||
| index: i, |
There was a problem hiding this comment.
is there a way to "add by index"? this index here should be the index of the wallet in the derivation path, not it's index in the array
There was a problem hiding this comment.
The index in this view actually relates to the derivation path in Lattice. If there's no override for fetchAddresses starting path, then it starts from address index 0.
|
Here's the packed extension for this build: |
|
Closing in favor of #1538 to run CI and merge this week |
Fixes BX-####
Figma link (if any):
What changed (plus any additional context for devs)
gridplus-sdkdependency and allowed it in lavamoat.semi-automaticE2E test for the GridPlus onboarding.Screen recordings / screenshots
What to test