Skip to content
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.

Sign up for GitHub

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(jellyfish-api-core): add wallet listWallets RPC #953

Merged
merged 5 commits into from
Feb 21, 2022
Merged

feat(jellyfish-api-core): add wallet listWallets RPC #953

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c802e5b
add list wallets rpc
kodemon Jan 6, 2022
add7b3a
use testing package for container creation
kodemon Jan 12, 2022
e997a38
tweak value assignment as per suggestion
kodemon Feb 18, 2022
8e6b9e1
split wallets generation and expect test
kodemon Feb 21, 2022
24bc79d
further update tests based on feedback
kodemon Feb 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions docs/node/CATEGORIES/05-wallet.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,3 +385,15 @@ interface wallet {
importPrivKey (privkey: string, label: string = "", rescan: boolean = true): Promise<void>
}
```

## listWallets

Returns a list of currently loaded wallets.

For full information on the wallet, use [getWalletInfo](#getwalletinfo)

```ts title="client.wallet.listWallets()"
interface wallet {
listWallets (): Promise<string[]>
}
```
39 changes: 39 additions & 0 deletions packages/jellyfish-api-core/__tests__/category/wallet/listWallets.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { Testing } from '@defichain/jellyfish-testing'
import { MasterNodeRegTestContainer, RegTestContainer } from '@defichain/testcontainers'
import { ContainerAdapterClient } from '../../container_adapter_client'

describe('Wallet without masternode', () => {
const container = new RegTestContainer()
const client = new ContainerAdapterClient(container)
kodemon marked this conversation as resolved.
Show resolved Hide resolved
fuxingloh marked this conversation as resolved.
Show resolved Hide resolved

beforeAll(async () => {
await container.start()
await client.wallet.createWallet('test')
})

afterAll(async () => {
await container.stop()
})

it('should listWallets', async () => {
const wallets = await client.wallet.listWallets()
expect(wallets).toStrictEqual(['', 'test'])
})
})

describe('Wallet with masternode', () => {
const { container, rpc: { wallet } } = Testing.create(new MasterNodeRegTestContainer())

beforeAll(async () => {
await container.start()
await wallet.createWallet('test')
})

afterAll(async () => {
await container.stop()
})

it('should listWallets', async () => {
await expect(wallet.listWallets()).resolves.toEqual(['', 'test'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Destructuring Assignment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for the feedback and I agree with your views on this. Will update the tests to reflect this pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated tests, thanks again for the input on this 👍🏻

})
})
10 changes: 10 additions & 0 deletions packages/jellyfish-api-core/src/category/wallet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,16 @@ export class Wallet {
async getTransaction (txid: string, includeWatchOnly: boolean = true): Promise<InWalletTransaction> {
return await this.client.call('gettransaction', [txid, includeWatchOnly], { amount: 'bignumber' })
}

/**
* Returns a list of currently loaded wallets.
* For full information on the wallet, use 'getwalletinfo'
*
* @return {Promise<string[]>}
*/
async listWallets (): Promise<string[]> {
return await this.client.call('listwallets', [], 'number')
}
}

export interface UTXO {
Expand Down