Adds Create Account flow; Snapshot Download updates #28
Conversation
| this.rpcClientPromise.resolve(rpcClient); | ||
| } | ||
|
|
||
| async start() { |
There was a problem hiding this comment.
I updated the start command so that init creates the Ironfish FullNode, so we can query for accounts on startup.
Now, start just starts the node.
| @@ -0,0 +1,29 @@ | |||
| import { useCallback, useMemo, useState } from "react"; | |||
|
|
|||
| export function useHasGroupBlur({ delay = 100 }: { delay: number }) { | |||
There was a problem hiding this comment.
Can this be deleted? Looks like a similar implementation is in formUtils already
| @@ -112,5 +110,6 @@ export class SnapshotManager { | |||
|
|
|||
| await downloadedSnapshot.replaceDatabase(); | |||
| await fsAsync.rm(downloadedSnapshot.file); | |||
| await node.openDB(); | |||
There was a problem hiding this comment.
Thinking about the pattern we want here for the node. There's 2 steps that require the node database to be open before we start the node: snapshot and getAccounts.
In this PR we open the node initially in Ironfish.init and then pass it to the snapshot to close it and then make sure it's open when it finishes. I could see it this way. I could also see not having the node open initially and then each individual process snapshot and getAccounts create their own node instance, open it and make sure it's close when its finished.
Personally a process opening its own node and making sure to close down at the end makes a little more sense to me than getting an open node and then making sure it remains open at the end. Wdyt?
There was a problem hiding this comment.
If each step opened their own node we could remove the fullNode promise. But then we'd also have to create a 2 memory clients. One ephemeral one to get the accounts and then another one on ironfish.start() So guess there are pros/cons to each approach
There was a problem hiding this comment.
I think I might want to refactor snapshotManager to take an Ironfish (the node app class) instance rather than a separate SDK and node. I think my thoughts here are similar to yours, but with different reasoning. It generally seems like it leads to confusing code if we have operations on the node itself (not through the RPC client) spread throughout the codebase, because you have to handle a bunch of node states, like whether the DB is open and whether the node is stopped/started.
Since the DB can only be held by one node at a time, I think the node instance naturally tends towards being a singleton, vs having per-process nodes in e.g. snapshot and getAccounts.
As an aside, we end up needing the fullNode promise for shutting down the node (and I think it’s generally worthwhile to hold an explicit handle to the node, since it’s running in the background anyway)
TL;DR, I think you're right that each step should make sure the node is in the necessary state, rather than the prior step needing to leave the node in a particular state. But I don't really want to slow this PR down unless it's causing bugs -- I figure I can go refactor it pretty easily if we run into issues at that point.
There was a problem hiding this comment.
Yea agreed. Talked about this and I think keeping the fullNode as a singleton on Ironfish makes sense
This adds the functionality (i.e. it doesn't look super pertty) for the initial create account flow.
It also updates the snapshot download process, so that it's part of the onboarding flow rather than a modal.