Add `wallet.sendMany` RPC #269

surangap · 2021-05-21T04:59:37Z

What kind of PR is this?:

/kind feature

What this PR does / why we need it:

codeclimate · 2021-05-21T04:59:56Z

Code Climate has analyzed commit 17ac805 and detected 0 issues on this pull request.

View more on Code Climate.

netlify · 2021-05-21T05:00:28Z

Deploy Preview for jellyfish-defi ready!

Built with commit 17ac805

https://deploy-preview-269--jellyfish-defi.netlify.app

canonbrother · 2021-05-21T05:03:12Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+    await container.stop()
+  })
+
+  // util functions NOTE(sp) : may be move to a common util file if possible ??


yea we actually have it in @defichain/testing

@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.

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

github-actions · 2021-05-21T05:03:33Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
packages/jellyfish/dist/jellyfish.umd.js	19.16 KB (+0.38% 🔺)	384 ms (+0.38% 🔺)	178 ms (+6.6% 🔺)	561 ms

codecov · 2021-05-21T05:15:53Z

Codecov Report

Merging #269 (a43cbe4) into main (cfbed46) will increase coverage by 0.24%.
The diff coverage is 100.00%.

❗ Current head a43cbe4 differs from pull request most recent head 17ac805. Consider uploading reports for the commit 17ac805 to get more accurate results

@@            Coverage Diff             @@
##             main     #269      +/-   ##
==========================================
+ Coverage   96.37%   96.62%   +0.24%     
==========================================
  Files          82       87       +5     
  Lines        2153     2308     +155     
  Branches      275      297      +22     
==========================================
+ Hits         2075     2230     +155     
  Misses         78       78

Impacted Files	Coverage Δ
...ish-api-core/__tests__/container_adapter_client.ts	`100.00% <100.00%> (ø)`
...ages/jellyfish-api-core/src/category/blockchain.ts	`100.00% <100.00%> (ø)`
packages/jellyfish-api-core/src/category/mining.ts	`100.00% <100.00%> (ø)`
packages/jellyfish-api-core/src/category/wallet.ts	`100.00% <100.00%> (ø)`
packages/jellyfish-api-core/src/index.ts	`100.00% <100.00%> (ø)`
packages/jellyfish-api-jsonrpc/src/index.ts	`97.43% <100.00%> (ø)`
...ackages/jellyfish-transaction-builder/src/index.ts	`100.00% <100.00%> (ø)`
...transaction-builder/src/txn/txn_builder_account.ts	`100.00% <100.00%> (ø)`
...h-transaction-builder/src/txn/txn_builder_error.ts	`100.00% <100.00%> (ø)`
...ransaction-builder/src/txn/txn_builder_liq_pool.ts	`100.00% <100.00%> (ø)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfbed46...17ac805. Read the comment docs.

canonbrother · 2021-05-21T05:20:52Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

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


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

yes. wil do that.

@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

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

canonbrother · 2021-05-21T05:28:09Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+
+      expect(typeof transactionId).toBe('string')
+
+      // NOTE(sp): How to test the effect of confTarget


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

@surangap btw we use this structure for TODO comments

https://github.com/DeFiCh/jellyfish/blob/main/CONTRIBUTING.md#todo-comments

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.

canonbrother

finish the note to ensure the test completeness
btw like your code style

jingyi2811 · 2021-05-21T05:45:59Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+      expect(utxos.length).toBe(1)
+      utxos.forEach(utxo => {
+        expect(utxo.address).toBe('mwsZw8nF7pKxWH8eoKL9tPxTpaFkz7QeLU')
+        expect(utxo.amount.isEqualTo(0.00001)).toBe(true)


toStrictEqual

jingyi2811 · 2021-05-21T05:46:43Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+   * Returns matching utxos for given transaction id and address.
+   *
+   * @param {string} txId transaction id
+   * @param {string} address address


1 address is enough

don't really need JSDoc for testing.

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

It's okay. You may remove the JSDoc.

jingyi2811 · 2021-05-21T05:47:21Z

packages/jellyfish-api-core/src/category/wallet.ts

@@ -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


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

jingyi2811 · 2021-05-21T05:51:19Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+      // In this case the we should only have one matching utxo
+      expect(utxos2.length).toBe(1)
+      utxos2.forEach(utxo => {
+        expect(utxo.address).toBe('mswsMVsyGMj1FzDMbbxw2QW3KvQAv2FKiy')


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

fuxingloh

I haven't review everything but added some comments

fuxingloh · 2021-05-21T13:32:29Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+
+      expect(typeof transactionId).toBe('string')
+
+      // NOTE(sp): How to test the effect of confTarget


@surangap btw we use this structure for TODO comments

https://github.com/DeFiCh/jellyfish/blob/main/CONTRIBUTING.md#todo-comments

fuxingloh · 2021-05-21T13:34:28Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

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


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.

fuxingloh · 2021-05-21T13:35:06Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+    return matchingUTXOs
+  }
+
+  //= ==============================================================================================


remove this

fuxingloh · 2021-05-21T13:37:52Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

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


@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

fuxingloh · 2021-05-21T13:40:13Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+
+      expect(typeof transactionId).toBe('string')
+
+      // NOTE(sp): How to test the effect of confTarget


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.

fuxingloh · 2021-05-21T13:42:13Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+   * Returns matching utxos for given transaction id and address.
+   *
+   * @param {string} txId transaction id
+   * @param {string} address address


don't really need JSDoc for testing.

fuxingloh · 2021-05-21T13:47:11Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+    await container.stop()
+  })
+
+  // util functions NOTE(sp) : may be move to a common util file if possible ??


@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.

fuxingloh · 2021-05-21T13:47:54Z

packages/jellyfish-api-core/__tests__/category/wallet.test.ts

+    await container.stop()
+  })
+
+  // util functions NOTE(sp) : may be move to a common util file if possible ??


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

fuxingloh

1 minor change

website/docs/jellyfish/api/wallet.md

surangap added 3 commits May 20, 2021 15:19

Added sendmany RPC

106849b

Updated sendMany RPC amounts param to do proper type checking

3a5aabe

Added tests for sendMany RPC

aa0630f

surangap requested review from canonbrother, fuxingloh and jingyi2811 as code owners May 21, 2021 04:59

defichain-bot added kind/feature New feature request area/jellyfish-api-core labels May 21, 2021

fuxingloh changed the title ~~Surangap/rpc sendmany~~ added sendmany rpc May 21, 2021

canonbrother reviewed May 21, 2021

View reviewed changes

packages/jellyfish-api-core/__tests__/category/wallet.test.ts Outdated Show resolved Hide resolved

canonbrother reviewed May 21, 2021

View reviewed changes

canonbrother suggested changes May 21, 2021

View reviewed changes

jingyi2811 reviewed May 21, 2021

View reviewed changes

fuxingloh requested changes May 21, 2021

View reviewed changes

fuxingloh marked this pull request as draft May 21, 2021 13:48

fuxingloh mentioned this pull request May 23, 2021

DeFi Blockchain RPC Implementations #48

Open

Refactored Code as per review suggestions.

1f0043b

surangap marked this pull request as ready for review May 24, 2021 09:25

surangap requested a review from thedoublejay as a code owner May 24, 2021 09:25

fuxingloh requested changes May 24, 2021

View reviewed changes

website/docs/jellyfish/api/wallet.md Show resolved Hide resolved

fuxingloh changed the title ~~added sendmany rpc~~ Add wallet.sendMany RPC May 24, 2021

fuxingloh marked this pull request as draft May 24, 2021 16:01

Added SendManyOptions estimateMode enum Mode to the docs

17ac805

fuxingloh approved these changes May 25, 2021

View reviewed changes

surangap marked this pull request as ready for review May 25, 2021 03:36

fuxingloh requested review from canonbrother and jingyi2811 May 25, 2021 03:36

fuxingloh enabled auto-merge (squash) May 25, 2021 03:36

canonbrother approved these changes May 25, 2021

View reviewed changes

fuxingloh merged commit f38efc3 into main May 25, 2021

fuxingloh deleted the surangap/rpc-sendmany branch May 25, 2021 03:46

canonbrother pushed a commit that referenced this pull request Jun 1, 2022

Bump @defichain dependencies (#269)

fd4044a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `wallet.sendMany` RPC #269

Add `wallet.sendMany` RPC #269

surangap commented May 21, 2021 •

edited by fuxingloh

Loading

codeclimate bot commented May 21, 2021 •

edited

Loading

netlify bot commented May 21, 2021 •

edited

Loading

canonbrother May 21, 2021

fuxingloh May 21, 2021

fuxingloh May 21, 2021

github-actions bot commented May 21, 2021 •

edited

Loading

codecov bot commented May 21, 2021 •

edited

Loading

canonbrother May 21, 2021

surangap May 21, 2021

fuxingloh May 21, 2021

canonbrother May 21, 2021

fuxingloh May 21, 2021

fuxingloh May 21, 2021

canonbrother left a comment

jingyi2811 May 21, 2021

jingyi2811 May 21, 2021

fuxingloh May 21, 2021

surangap May 24, 2021

jingyi2811 May 24, 2021 •

edited

Loading

jingyi2811 May 21, 2021

jingyi2811 May 21, 2021 •

edited

Loading

fuxingloh left a comment

fuxingloh May 21, 2021

fuxingloh May 21, 2021 •

edited

Loading

fuxingloh May 21, 2021

fuxingloh May 21, 2021

fuxingloh May 21, 2021

fuxingloh May 21, 2021

fuxingloh May 21, 2021

fuxingloh May 21, 2021

fuxingloh left a comment


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

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

Add wallet.sendMany RPC #269

Add wallet.sendMany RPC #269

Conversation

surangap commented May 21, 2021 • edited by fuxingloh Loading

What kind of PR is this?:

What this PR does / why we need it:

codeclimate bot commented May 21, 2021 • edited Loading

netlify bot commented May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 21, 2021 • edited Loading

size-limit report 📦

codecov bot commented May 21, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canonbrother left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingyi2811 May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingyi2811 May 21, 2021 • edited Loading

Choose a reason for hiding this comment

fuxingloh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuxingloh May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuxingloh left a comment

Choose a reason for hiding this comment

Add `wallet.sendMany` RPC #269

Add `wallet.sendMany` RPC #269

surangap commented May 21, 2021 •

edited by fuxingloh

Loading

codeclimate bot commented May 21, 2021 •

edited

Loading

netlify bot commented May 21, 2021 •

edited

Loading

github-actions bot commented May 21, 2021 •

edited

Loading

codecov bot commented May 21, 2021 •

edited

Loading

jingyi2811 May 24, 2021 •

edited

Loading

jingyi2811 May 21, 2021 •

edited

Loading

fuxingloh May 21, 2021 •

edited

Loading