-
Notifications
You must be signed in to change notification settings - Fork 204
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: allow for lazy wallet initialization #331
feat: allow for lazy wallet initialization #331
Conversation
Codecov Report
@@ Coverage Diff @@
## main #331 +/- ##
==========================================
- Coverage 88.21% 87.77% -0.45%
==========================================
Files 220 224 +4
Lines 4039 4113 +74
Branches 475 489 +14
==========================================
+ Hits 3563 3610 +47
- Misses 476 503 +27
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good to have some "init" kind of method simplifying a lot of stuff for a developer. On the other hand, it would be good to provide some sort of flexibility out of the box (which we don't have to address in this PR of course).
masterSecretId: string | ||
walletConfig: WalletConfig | ||
walletCredentials: WalletCredentials | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm note sure if this type has right cohesion. It seems these attributes are kind of unrealated, at least from the first point of view 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It contains related to an open wallet. I agree with your point, but would like to solve that in a separate PR.
I think we can make the wallet a bit more straightforward by splitsing the functionality in two parts:
- A "controller" or something that does the following
- create
- open
- delete
- close
- A wallet that does the actual wallet stuff. You need an open wallet to use it.
This will make it easier to handle multiple wallets I think and separates the lifecycle of a wallet from the usage of the wallet
I'd like to do that in a future PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, good idea 👍
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
fdf1d12
to
c2406ee
Compare
@jakubkoci made updates based on your feedback |
closeAndDeleteWallet
as it was only used for tests. They now inject the wallet to close and deletewalletConfig
andwalletCredentials
optional inInitConfig
walletConfig
andwalletCredentials
are available an error will be thrown