feat(jellyfish-api-core): add wallet listWallets RPC #953
Conversation
|
Code Climate has analyzed commit 24bc79d and detected 0 issues on this pull request. View more on Code Climate. |
Codecov Report
@@ Coverage Diff @@
## main #953 +/- ##
==========================================
- Coverage 94.69% 94.50% -0.20%
==========================================
Files 158 169 +11
Lines 4806 5037 +231
Branches 620 657 +37
==========================================
+ Hits 4551 4760 +209
- Misses 255 259 +4
- Partials 0 18 +18
Continue to review full report at Codecov.
|
kodemon
requested review from
canonbrother,
chee-chyuan,
fuxingloh,
ivan-zynesis,
jingyi2811 and
surangap
as code owners
packages/jellyfish-api-core/__tests__/category/wallet/listWallets.test.ts
Show resolved
Hide resolved
kodemon
dismissed stale reviews from chee-chyuan and canonbrother
via
add7b3a
|
✔️ Deploy Preview for jellyfish-defi ready! 🔨 Explore the source changes: e997a38 🔍 Inspect the deploy log: https://app.netlify.com/sites/jellyfish-defi/deploys/620fd9fe6b6e900007667439 😎 Browse the preview: https://deploy-preview-953--jellyfish-defi.netlify.app |
packages/jellyfish-api-core/__tests__/category/wallet/listWallets.test.ts
Show resolved
Hide resolved
packages/jellyfish-api-core/__tests__/category/wallet/listWallets.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Fuxing Loh <[email protected]>
| }) | ||
|
|
||
| it('should listWallets', async () => { | ||
| await expect(wallet.listWallets()).resolves.toEqual(['', 'test']) |
There was a problem hiding this comment.
done, what is your thoughts on https://github.com/DeFiCh/jellyfish/pull/953/files#diff-1045b44753a823e6c6779b841a5e8d54fb6286c2961837d95fb2232b711b7218R25 ?
Would you expect something more like
const master = new MasterNodeRegTestContainer()
const { container, rpc: { wallet } } = Testing.create(master)or
const master = new MasterNodeRegTestContainer()
const container = Testing.create(master)Personally I am more leaning towards the way its currently written, but would be good to align with expectations in Jellyfish for these sort of approaches.
Pulling out variables of objects like this was a pretty neat ES update and I find myself using it quite often in other projects for method arguments and variable assignment.
There was a problem hiding this comment.
IMO, destructuring is great in functional method parameters, but in tests, it's pretty counterintuitive as it introduces too much symbol memorization when one is trying to understand what is going on.
I am also towards reducing unnecessary symbols that are not part of the testing context. By only introducing symbols that are part of the testing context. Optimizing for test readability over anything else, as for writing test and writing component we should take a different approach.
E.g.
const master = new MasterNodeRegTestContainer()
const { container, rpc: { wallet } } = Testing.create(master)For the above, you expose 2 new symbols for the reader to track and understand as they are reading the test.
Vs.
const testing = Testing.create(new MasterNodeRegTestContainer())For the above, you only have 1 symbol which is the "testing" framework object for the reader to follow along.
Subsequently:
testing.container.generate(1)
testing.rpc.wallet.doSomething(1)It's contextually more understandable for the user to know that you are accessing the testing - container - to generate 1 block.
That's for tests. And it only makes sense for tests, as you are trying to make it as effortless as possible for the reviewer to understand what's going on. However for non-tests, I am in favor of destructuring as it reduces non-relevant symbols by focusing on what is required for each method/function.
There was a problem hiding this comment.
Great, thanks for the feedback and I agree with your views on this. Will update the tests to reflect this pattern.
There was a problem hiding this comment.
Updated tests, thanks again for the input on this 👍🏻
What this PR does / why we need it:
/kind feature
Which issue(s) does this PR fixes?:
Add listWallets rpc as part of ongoing #48