feat(jellyfish-api-core): add wallet.listtransactions RPC #2060

andyrobert3 · 2023-02-20T11:14:17Z

What this PR does / why we need it:

/kind feature

Which issue(s) does this PR fixes?:

Implemented RPC call for #48

wallet.listtransactions

Additional comments?:

Unsure how to test the "skip" parameter for the wallet.listtransactions function

codeclimate · 2023-02-20T11:14:48Z

Code Climate has analyzed commit 530b5f6 and detected 0 issues on this pull request.

View more on Code Climate.

netlify · 2023-02-20T11:14:56Z

✅ Deploy Preview for jellyfishsdk ready!

Name	Link
🔨 Latest commit	`530b5f6`
🔍 Latest deploy log	https://app.netlify.com/sites/jellyfishsdk/deploys/63f81aa19f1a7600082650bb
😎 Deploy Preview	https://deploy-preview-2060--jellyfishsdk.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codecov · 2023-02-20T14:47:53Z

Codecov Report

Base: 82.49% // Head: 91.58% // Increases project coverage by +9.08% 🎉

Coverage data is based on head (530b5f6) compared to base (b0642d9).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2060      +/-   ##
==========================================
+ Coverage   82.49%   91.58%   +9.08%     
==========================================
  Files         357      362       +5     
  Lines       10837    10884      +47     
  Branches     1416     1421       +5     
==========================================
+ Hits         8940     9968    +1028     
+ Misses       1803      873     -930     
+ Partials       94       43      -51

Impacted Files	Coverage Δ
packages/jellyfish-api-core/src/category/wallet.ts	`100.00% <100.00%> (+4.49%)`	⬆️
...ners/src/containers/RegTestContainer/Persistent.ts	`11.11% <0.00%> (-81.49%)`	⬇️
packages/whale-api-client/src/api/consortium.ts	`33.33% <0.00%> (-66.67%)`	⬇️
...es/ocean-api-client/src/errors/TimeoutException.ts	`33.33% <0.00%> (-66.67%)`	⬇️
...ckages/jellyfish-network/src/TransactionSkipped.ts	`37.50% <0.00%> (-50.00%)`	⬇️
...core/src/saga/AddressParser/dftx/AccountToUtxos.ts	`62.50% <0.00%> (-37.50%)`	⬇️
...ges/rich-list-core/src/saga/AddressParser/index.ts	`60.71% <0.00%> (-35.72%)`	⬇️
...ule.indexer/model/dftx/set.oracle.data.interval.ts	`61.40% <0.00%> (-35.09%)`	⬇️
...pi/src/module.indexer/model/dftx/appoint.oracle.ts	`75.75% <0.00%> (-24.25%)`	⬇️
...api/src/module.indexer/model/dftx/update.oracle.ts	`74.41% <0.00%> (-23.26%)`	⬇️
... and 110 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

eli-lim

Generally LGTM! Some comments

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

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

eli-lim

Generally LGTM

eli-lim · 2023-02-22T06:51:39Z

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

+    expect(inWalletTransactions.length).toStrictEqual(10)
+
+    for (const inWalletTransaction of inWalletTransactions) {
+      expect(inWalletTransaction).toMatchObject({


Is there a reason why we prefer toMatchObject here over toStrictEqual?

Here are the specifications on what attributes are returned by the listtransactions function on DeFiChain

https://github.com/DeFiCh/ain/blob/master/src/wallet/rpcwallet.cpp

RPCResult { "[\n" " {\n" " \"address\":\"address\", (string) The defi address of the transaction.\n" " \"category\": (string) The transaction category.\n" " \"send\" Transactions sent.\n" " \"receive\" Non-coinbase transactions received.\n" " \"generate\" Coinbase transactions received with more than 100 confirmations.\n" " \"immature\" Coinbase transactions received with 100 or fewer confirmations.\n" " \"orphan\" Orphaned coinbase transactions received.\n" " \"amount\": x.xxx, (numeric) The amount in " + CURRENCY_UNIT + ". This is negative for the 'send' category, and is positive\n" " for all other categories\n" " \"label\": \"label\", (string) A comment for the address/transaction, if any\n" " \"vout\": n, (numeric) the vout value\n" " \"fee\": x.xxx, (numeric) The amount of the fee in " + CURRENCY_UNIT + ". This is negative and only available for the \n" " 'send' category of transactions.\n" " \"confirmations\": n, (numeric) The number of confirmations for the transaction. Negative confirmations indicate the\n" " transaction conflicts with the block chain\n" " \"trusted\": xxx, (bool) Whether we consider the outputs of this unconfirmed transaction safe to spend.\n" " \"blockhash\": \"hashvalue\", (string) The block hash containing the transaction.\n" " \"blockindex\": n, (numeric) The index of the transaction in the block that includes it.\n" " \"blocktime\": xxx, (numeric) The block time in seconds since epoch (1 Jan 1970 GMT).\n" " \"txid\": \"transactionid\", (string) The transaction id.\n" " \"time\": xxx, (numeric) The transaction time in seconds since epoch (midnight Jan 1 1970 GMT).\n" " \"timereceived\": xxx, (numeric) The time received in seconds since epoch (midnight Jan 1 1970 GMT).\n" " \"comment\": \"...\", (string) If a comment is associated with the transaction.\n" " \"bip125-replaceable\": \"yes|no|unknown\", (string) Whether this transaction could be replaced due to BIP125 (replace-by-fee);\n" " may be unknown for unconfirmed transactions not in the mempool\n" " \"abandoned\": xxx (bool) 'true' if the transaction has been abandoned (inputs are respendable). Only available for the \n" " 'send' category of transactions.\n" " }\n" "]\n" }

When running the tests, sometimes there are additional optional returned parameters that are not included in the specifications above:

generated -> boolean

walletconflicts -> Array

These parameters appear in some responses, and not others.

Using toStrictEqual may fail tests here, the extra parameters generated & walletconflicts are not always included in the response.

However, toMatchObject is defined as "check that a JavaScript object matches a subset of the properties of an object"

In this case, we can just compare the response result if it matches a subset of the specification parameters from the DeFiChain repository

If anyone from the DeFi chain team knows when & why the 2 extra parameters appear, feel free to add on here so I can modify my tests to use toStrictEqual

Ah okay that makes sense

I guess the RPC doc needs updating in ain then

do you know who should i ask here? @eli-lim

I'll probably raise a ticket to the ain repo?

Sure, just raise a ticket

In the mean time, let's wait for others to review this PR

andyrobert3 added 4 commits February 20, 2023 19:05

chore: add documentation for listTransactions

9b48200

feat: add service logic for listTransactions

3a0f810

chore: add test logic for listTransactions

bee26f5

chore: remove irrelevant comment

4c34de3

github-actions bot added apps/website area/packages kind/feature New feature request labels Feb 20, 2023

andyrobert3 marked this pull request as ready for review February 21, 2023 06:28

andyrobert3 requested review from fuxingloh, canonbrother and ivan-zynesis as code owners February 21, 2023 06:28

eli-lim reviewed Feb 22, 2023

View reviewed changes

andyrobert3 added 7 commits February 22, 2023 10:18

Merge branch 'main' into main

e407451

feat: Replace "listTransactions" parameters into object

05d3e5d

feat: Update tests to be more readable

1e9b101

fix: Increase test case condition precision

4e7c683

chore: Update tests to be more strict in equality comparison

2c6113a

chore: Replace async for loop with Promise.all

cb14e37

fix: Update .md file for listTransactions

be68dea

andyrobert3 requested a review from eli-lim February 22, 2023 06:18

eli-lim approved these changes Feb 22, 2023

View reviewed changes

Merge branch 'main' into main

530b5f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jellyfish-api-core): add wallet.listtransactions RPC #2060

feat(jellyfish-api-core): add wallet.listtransactions RPC #2060

andyrobert3 commented Feb 20, 2023 •

edited

Loading

codeclimate bot commented Feb 20, 2023 •

edited

Loading

netlify bot commented Feb 20, 2023 •

edited

Loading

codecov bot commented Feb 20, 2023 •

edited

Loading

eli-lim left a comment

eli-lim left a comment

eli-lim Feb 22, 2023

andyrobert3 Feb 22, 2023 •

edited

Loading

andyrobert3 Feb 23, 2023

eli-lim Feb 24, 2023

andyrobert3 Feb 24, 2023

eli-lim Feb 24, 2023

feat(jellyfish-api-core): add wallet.listtransactions RPC #2060

Are you sure you want to change the base?

feat(jellyfish-api-core): add wallet.listtransactions RPC #2060

Conversation

andyrobert3 commented Feb 20, 2023 • edited Loading

What this PR does / why we need it:

Which issue(s) does this PR fixes?:

Additional comments?:

codeclimate bot commented Feb 20, 2023 • edited Loading

netlify bot commented Feb 20, 2023 • edited Loading

✅ Deploy Preview for jellyfishsdk ready!

codecov bot commented Feb 20, 2023 • edited Loading

Codecov Report

eli-lim left a comment

Choose a reason for hiding this comment

eli-lim left a comment

Choose a reason for hiding this comment

eli-lim Feb 22, 2023

Choose a reason for hiding this comment

andyrobert3 Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

andyrobert3 Feb 23, 2023

Choose a reason for hiding this comment

eli-lim Feb 24, 2023

Choose a reason for hiding this comment

andyrobert3 Feb 24, 2023

Choose a reason for hiding this comment

eli-lim Feb 24, 2023

Choose a reason for hiding this comment

andyrobert3 commented Feb 20, 2023 •

edited

Loading

codeclimate bot commented Feb 20, 2023 •

edited

Loading

netlify bot commented Feb 20, 2023 •

edited

Loading

codecov bot commented Feb 20, 2023 •

edited

Loading

andyrobert3 Feb 22, 2023 •

edited

Loading