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.

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

Add wallet.sendMany RPC #269

Merged
merged 5 commits into from
May 25, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
184 changes: 183 additions & 1 deletion packages/jellyfish-api-core/__tests__/category/wallet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { ContainerAdapterClient } from '../container_adapter_client'
import { MasterNodeRegTestContainer, RegTestContainer } from '@defichain/testcontainers'
import { BigNumber, wallet } from '../../src'
import waitForExpect from 'wait-for-expect'
import { UTXO, ListUnspentOptions, WalletFlag, SendToAddressOptions, Mode } from '../../src/category/wallet'
import { UTXO, ListUnspentOptions, WalletFlag, SendToAddressOptions, Mode, SendManyOptions } from '../../src/category/wallet'

describe('non masternode', () => {
const container = new RegTestContainer()
Expand Down Expand Up @@ -632,3 +632,185 @@ describe('masternode', () => {
})
})
})

describe('sendMany', () => {
// NOTE(sp): defid side(c++) does not have much tests for sendmany RPC atm.
const container = new MasterNodeRegTestContainer()
const client = new ContainerAdapterClient(container)

beforeAll(async () => {
await container.start()
await container.waitForReady()
await container.waitForWalletCoinbaseMaturity()
})

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

// util functions NOTE(sp) : may be move to a common util file if possible ??
Copy link
Contributor

Choose a reason for hiding this comment

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

yea we actually have it in @defichain/testing

Copy link
Contributor

Choose a reason for hiding this comment

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

@surangap you can keep it in this file as it is.

for api-core we try not to add too many test utils boilerplate (files) to keep the test scoped down for brevity. For other packages, e.g. the more complex use cases we have and uses a lot of testing utils. @canonbrother is maintaining the testing utilities now.

Copy link
Contributor

Choose a reason for hiding this comment

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

We trying to contain the code debt so that it doesn't spaghetti around each other.

/**
* Returns matching utxos for given transaction id and address.
*
* @param {string} txId transaction id
* @param {string} address address
Copy link
Contributor

Choose a reason for hiding this comment

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

1 address is enough

Copy link
Contributor

Choose a reason for hiding this comment

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

don't really need JSDoc for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jingyi2811 didn't quite understand your comment. input params for the function are txId and address

Copy link
Contributor

@jingyi2811 jingyi2811 May 24, 2021

Choose a reason for hiding this comment

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

It's okay. You may remove the JSDoc.

* @return {Promise<UTXO[]>} matching UTXO[]
*/
const getMatchingUTXO = async (txId: string, address: string): Promise<UTXO[]> => {
const options: ListUnspentOptions = {
addresses: [address]
}
const matchingUTXOs: UTXO[] = []

const listUnspent = async (): Promise<void> => {
const utxos: UTXO[] = await client.wallet.listUnspent(1, 9999999, options)
utxos.forEach(utxo => {
expect(utxo.address).toBe(address)
if (utxo.txid === txId) {
matchingUTXOs.push(utxo)
}
})
}
await listUnspent()
return matchingUTXOs
}

//= ==============================================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

it('should getBalance >= 100', async () => {
return await waitForExpect(async () => {
const balance: BigNumber = await client.wallet.getBalance()
expect(balance.isGreaterThan(new BigNumber('100'))).toBe(true)
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a part of test
suggestion here is to park this assertion on beforeAll()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. wil do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@surangap container has much more testing tools available now. You can take a look at those implementations and use them. If you need to add more please create PR for them.

https://github.com/DeFiCh/jellyfish/blob/63fd7700ef9959cc50bb2dadfc3cff202771aaab/packages/testcontainers/src/chains/reg_test_container/masternode.ts#L105-L114


it('should send one address using sendMany', async () => {
const amounts: Record<string, number> = { mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU: 0.00001 }
return await waitForExpect(async () => {
const transactionId = await client.wallet.sendMany(amounts)
expect(typeof transactionId).toBe('string')

// generate one block
await container.generate(1)

// check the corresponding UTXO
const utxos = await getMatchingUTXO(transactionId, 'mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU')
// In this case the we should only have one matching utxo
expect(utxos.length).toBe(1)
utxos.forEach(utxo => {
expect(utxo.address).toBe('mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU')
expect(utxo.amount.isEqualTo(0.00001)).toBe(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

toStrictEqual

})
})
})

it('should send multiple address using sendMany', async () => {
const amounts: Record<string, number> = { mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU: 0.00001, mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy: 0.00002 }
return await waitForExpect(async () => {
const transactionId = await client.wallet.sendMany(amounts)
expect(typeof transactionId).toBe('string')

// generate one block
await container.generate(1)

// check the corresponding UTXOs
const utxos = await getMatchingUTXO(transactionId, 'mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU')
// In this case the we should only have one matching utxo
expect(utxos.length).toBe(1)
utxos.forEach(utxo => {
expect(utxo.address).toBe('mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU')
expect(utxo.amount.isEqualTo(0.00001)).toBe(true)
})

const utxos2 = await getMatchingUTXO(transactionId, 'mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy')
// In this case the we should only have one matching utxo
expect(utxos2.length).toBe(1)
utxos2.forEach(utxo => {
expect(utxo.address).toBe('mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy')
expect(utxo.amount.isEqualTo(0.00002)).toBe(true)
})
})
})

it('should sendMany with comment', async () => {
const amounts: Record<string, number> = { mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU: 0.00001, mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy: 0.00002 }
return await waitForExpect(async () => {
const options: SendManyOptions = {
comment: 'test comment'
}
const transactionId = await client.wallet.sendMany(amounts, [], options)

expect(typeof transactionId).toBe('string')

// NOTE(sp): How to test the comment is there and where
surangap marked this conversation as resolved.
Show resolved Hide resolved
})
})

it('should sendMany with replaceable', async () => {
const amounts: Record<string, number> = { mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU: 0.00001, mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy: 0.00002 }
return await waitForExpect(async () => {
const options: SendManyOptions = {
replaceable: true
}
const transactionId = await client.wallet.sendMany(amounts, [], options)

expect(typeof transactionId).toBe('string')
})
})

it('should sendMany with confTarget', async () => {
const amounts: Record<string, number> = { mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU: 0.00001, mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy: 0.00002 }
return await waitForExpect(async () => {
Copy link
Contributor

@fuxingloh fuxingloh May 21, 2021

Choose a reason for hiding this comment

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

I think we don't need to use waitForExpect at all for sendMany tests, our setup should be deterministic now 🤞. Previously we had mempool conflict issues and a bunch of other flaky reasons. All those are fixed now. By using waitForExpect we can't detect race conditions error which is bad.

const options: SendManyOptions = {
confTarget: 60
}
const transactionId = await client.wallet.sendMany(amounts, [], options)

expect(typeof transactionId).toBe('string')

// NOTE(sp): How to test the effect of confTarget
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps try getrawtransaction -> generate block -> getblock with minconf

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

confTarget is likely fees testing, meaning calculate the fee that confirms in 60 blocks. But you need to create 16MB of transactions to test that. Event mainnet don't hit that cap, so you don't need to test that behavior.

})
})

it('should sendMany with estimateMode', async () => {
const amounts: Record<string, number> = { mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU: 0.00001, mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy: 0.00002 }
return await waitForExpect(async () => {
const options: SendManyOptions = {
estimateMode: Mode.ECONOMICAL
}
const transactionId = await client.wallet.sendMany(amounts, [], options)

expect(typeof transactionId).toBe('string')
})
})

it('should sendMany with fee substracted from mentioned recipients', async () => {
const amounts: Record<string, number> = { mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU: 0.00001, mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy: 10.5 }
const subtractFeeFrom: string [] = ['mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy']
return await waitForExpect(async () => {
const transactionId = await client.wallet.sendMany(amounts, subtractFeeFrom)
expect(typeof transactionId).toBe('string')

// generate one block
await container.generate(1)

// check the corresponding UTXOs
const utxos = await getMatchingUTXO(transactionId, 'mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU')
// In this case the we should only have one matching utxo
expect(utxos.length).toBe(1)
utxos.forEach(utxo => {
expect(utxo.address).toBe('mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU')
// amount should be equal to 0.00001
expect(utxo.amount.isEqualTo(0.00001)).toBe(true)
})

const utxos2 = await getMatchingUTXO(transactionId, 'mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy')
// In this case the we should only have one matching utxo
expect(utxos2.length).toBe(1)
utxos2.forEach(utxo => {
expect(utxo.address).toBe('mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy')
Copy link
Contributor

@jingyi2811 jingyi2811 May 21, 2021

Choose a reason for hiding this comment

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

Like what CanonBrother told me just now, change toBe to toStrictEqual

// amount should be less than 10.5
expect(utxo.amount.isLessThan(10.5)).toBe(true)
})
})
})
})
43 changes: 43 additions & 0 deletions packages/jellyfish-api-core/src/category/wallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,42 @@ export class Wallet {
async listAddressGroupings (): Promise<any[][][]> {
return await this.client.call('listaddressgroupings', [], 'bignumber')
}

/**
* Send given amounts to multiple given address and return a transaction id
Copy link
Contributor

Choose a reason for hiding this comment

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

Like what you wrote in md file. Add a fullstop.

*
* @param {Record<string, number>} amounts Dictionary/map with individual addresses and amounts
* @param {string[]} subtractfeefrom Array of addresses from which fee needs to be deducted.
* @param {SendManyOptions} options
* @param {string} [options.comment] A comment
* @param {boolean} [options.replaceable] Allow this transaction to be replaced by a transaction with higher fees via BIP 125
* @param {number} [options.confTarget] Confirmation target (in blocks)
* @param {Mode} [options.estimateMode] The fee estimate mode, must be one of (Mode.UNSET, Mode.ECONOMICAL, Mode.CONSERVATIVE)
* @return {Promise<string>} hex string of the transaction
*/
async sendMany (
amounts: Record<string, number>,
subtractfeefrom: string [] = [],
options: SendManyOptions = {}): Promise<string> {
const {
comment = '',
replaceable = false,
confTarget = 6,
estimateMode = Mode.UNSET
} = options

const dummy: string = '' // Must be set to '' for backward compatibality.
const minconf: number = 0 // Ignored dummy value

return await this.client.call(
'sendmany',
[
dummy, amounts, minconf, comment, subtractfeefrom,
replaceable, confTarget, estimateMode
],
'bignumber'
)
}
}

export interface UTXO {
Expand Down Expand Up @@ -263,6 +299,13 @@ export interface SendToAddressOptions {
avoidReuse?: boolean
}

export interface SendManyOptions {
comment?: string
replaceable?: boolean
confTarget?: number
estimateMode?: Mode
}

export interface CreateWalletResult {
name: string
warning: string
Expand Down
17 changes: 17 additions & 0 deletions website/docs/jellyfish/api/wallet.md
Original file line number Diff line number Diff line change
Expand Up @@ -256,3 +256,20 @@ interface wallet {
listAddressGroupings (): Promise<any[][][]>
}
```

## sendMany

Send given amounts to multiple given address and return a transaction id.

```ts title="client.wallet.sendMany()"
interface wallet {
async sendMany (amounts: Record<string, number> , subtractfeefrom: string [] = [], options: SendManyOptions = {}): Promise<string>
}

interface SendManyOptions {
comment?: string
replaceable?: boolean
confTarget?: number
estimateMode?: Mode
surangap marked this conversation as resolved.
Show resolved Hide resolved
}
```