Feat/add gridplus lattice1 #3744
Conversation
There was a problem hiding this comment.
Did a final pass here, I think a couple of questions remain on my end:
- Am I reading this right that this does not accommodate multiple Lattice1s? The data model in the service does not seem to allow for that, but I could be misreading. It needs to be massaged anyway (from local storage to IndexedDB), so it's not a massive deal.
- Does the current setup assume that the Lattice1 will always be connected? In particular, it seems like it doesn't handle connection status, and my understanding is the Lattice1 is communicated with over the internet—so a failure in the wifi router, having the Lattice1 off, etc, could result in the Lattice1 not actually being available.
There are two notes below that requires some work I'll be doing (one is about renaming from gridplus to grid-plus or gridPlus and Gridplus to GridPlus, depending on where we are, just for consistency; the other is about a specific aspect of internal-ethereum-provider). The rest I don't think is a blocker, and we can merge & release with future iterations of work addressing the remainder.
| } | ||
|
|
||
| const gridplusSlice = createSlice({ | ||
| name: "gridplus", |
There was a problem hiding this comment.
Everywhere we refer to gridplus should be grid-plus, including filenames, as capital letters/separate words translate to - when we move outside of camel-casing. Anywhere we then import gridplus should be gridPlus, etc, for consistency.
| @@ -503,10 +503,13 @@ export default class InternalEthereumProviderService extends BaseService<Events> | |||
| // Ethers does not want to see the EIP712Domain field, extract it. | |||
| const { EIP712Domain, ...typesForSigning } = params.typedData.types | |||
|
|
|||
| const { domain } = params.typedData | |||
| domain.chainId = params.account.network.chainID | |||
There was a problem hiding this comment.
Reading EIP-712, what should be happening here is that the signer should fail a signature with a mismatch between the network id and the current active network. In practice that means we should drop this step and instead fail at the callsite, line 179, or move the callsite's account resolution into here and fail in here in that case, only passing the JSON param from there.
I can make this change.
There was a problem hiding this comment.
Hm, reading further, we actually catch this in the SigningService right now, where we check the account network chainID against the chainID passed in the data:
extension/background/services/signing/index.ts
Lines 244 to 260 in 34d1350
With this in mind, the above code actually circumvents this explicit error in favor of an implicit one (an invalid signature), so I'll be removing this code and leaving the current paths unchanged; please let me know if there was a strong reason for this adjustment, though.
| @@ -21,6 +21,8 @@ const getSignerRecordId = (signer: AccountSignerWithId): SignerRecordId => { | |||
| return `${signer.type}/${signer.keyringID}` | |||
| case "private-key": | |||
| return `${signer.type}/${signer.walletID}` | |||
| case "gridplus": | |||
| return `${signer.type}/${signer.path}` | |||
There was a problem hiding this comment.
Note that the purpose here is to distinguish “signers” in the sense of accounts that would be derived from the same place. For Ledger, each “device” would get its own signer. I'm worried that path here refers to a full derivation path, which would mean each address would get its own “signer”. That results in poor UX because each signer gets a grouping in the selector list, with accounts getting entries under the signer; for example, here, “Taho 1” is a signer, and testertesting.eth is an account:
If every account added from a GridPlus device corresponds to a grouping, that'll potentially create a lot of clutter—instead.
Ultimately right now this is only used to manage that category title (“Taho 1” in the example above), so it's not a huge blocker—but it may be worth considering if it would be better to funnel the device id out of the service so that it can be used for differentiation here like it is for Ledgers.
|
|
||
| async readClient() { | ||
| this.client = | ||
| (await storage.local.get(CLIENT_STORAGE_KEY))?.[CLIENT_STORAGE_KEY] ?? |
There was a problem hiding this comment.
Services generally don't read or write from local storage—that's considered only for ephemeral data. Services instead own an IndexedDB database (see e.g. ledger/db.ts) that handles querying, etc.
I don't think this is a blocker, but it's a relatively important part of the underlying architecture as local storage has aggressive size quotas and can fail in unexpected or silent ways when those size quotas are exceeded, while IndexedDB has a much more generous quote and clearer failure modes.
| async readAddresses() { | ||
| const persistedAddresses = | ||
| (await storage.local.get(ADDRESSES_STORAGE_KEY))?.[ | ||
| ADDRESSES_STORAGE_KEY | ||
| ] ?? "[]" | ||
| const activeAddresses = JSON.parse(persistedAddresses) | ||
| this.activeAddresses = activeAddresses | ||
| return this.activeAddresses | ||
| } |
There was a problem hiding this comment.
Currently this happens in Popup.tsx via a hook, but instead it should happen during the internalStartService call that triggers when the service starts up (as the service's lifecycle is typically longer than the lifecycle of the popup).
| } | ||
|
|
||
| async pairDevice({ pairingCode }: { pairingCode: string }) { | ||
| await this.readClient() |
There was a problem hiding this comment.
What's the purpose of running this every time pairDevice, fetchAddresses, etc are run? It seems like it just reads data that's been serialized, but that data remains in-memory, no? I don't see it cleared anywhere.
Also, is this.client secret/sensitive data or is it irrelevant if someone else gets access to it?
Changes
Screenshots