fix(jellyfish-api-core): Update listGovProposalVotes arguments #1908

shohamc1 · 2022-12-07T10:14:38Z

What this PR does / why we need it:

Update listGovProposalVotes arguments as per Add pagination support for listgovproposalvotes ain#1635. This is required to keep compatibility with the next release.

codeclimate · 2022-12-07T10:15:14Z

Code Climate has analyzed commit 5c9ac5a and detected 0 issues on this pull request.

netlify · 2022-12-07T10:15:27Z

✅ Deploy Preview for jellyfishsdk ready!

Name	Link
🔨 Latest commit	`5c9ac5a`
🔍 Latest deploy log	https://app.netlify.com/sites/jellyfishsdk/deploys/63bd3665e26ca60008a37ee4
😎 Deploy Preview	https://deploy-preview-1908--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 · 2022-12-07T10:23:13Z

Codecov Report

Base: 89.61% // Head: 93.03% // Increases project coverage by +3.42% 🎉

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1908      +/-   ##
==========================================
+ Coverage   89.61%   93.03%   +3.42%     
==========================================
  Files         366      367       +1     
  Lines       10768    10772       +4     
  Branches     1378     1378              
==========================================
+ Hits         9650    10022     +372     
+ Misses       1064      714     -350     
+ Partials       54       36      -18

Impacted Files	Coverage Δ
...ages/jellyfish-api-core/src/category/governance.ts	`100.00% <100.00%> (ø)`
...atus-api/src/controllers/OracleStatusController.ts	`19.56% <0.00%> (-78.27%)`	⬇️
packages/jellyfish-address/src/p2wsh.ts	`48.27% <0.00%> (-51.73%)`	⬇️
apps/legacy-api/src/modules/WhaleApiModule.ts	`45.83% <0.00%> (-45.84%)`	⬇️
packages/jellyfish-wallet-mnemonic/src/mnemonic.ts	`63.63% <0.00%> (-36.37%)`	⬇️
...yfish-transaction/src/script/dftx/dftx_unmapped.ts	`77.77% <0.00%> (-22.23%)`	⬇️
...ish-transaction/src/script/dftx/dftx_governance.ts	`84.61% <0.00%> (-15.39%)`	⬇️
...s/jellyfish-transaction-builder/src/txn/txn_fee.ts	`89.28% <0.00%> (-10.72%)`	⬇️
apps/whale-api/src/module.api/fee.controller.ts	`90.00% <0.00%> (-10.00%)`	⬇️
packages/testcontainers/src/utils.ts	`91.66% <0.00%> (-8.34%)`	⬇️
... and 62 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.

github-actions · 2022-12-07T10:30:22Z

Docker build preview for jellyfish/apps is ready!

Built with commit c893dfc

ghcr.io/jellyfishsdk/legacy-api:pr-1908
ghcr.io/jellyfishsdk/ocean-api:pr-1908
ghcr.io/jellyfishsdk/playground-api:pr-1908
ghcr.io/jellyfishsdk/status-api:pr-1908
ghcr.io/jellyfishsdk/whale-api:pr-1908

You can also get an immutable image with the commit hash

ghcr.io/jellyfishsdk/legacy-api:c893dfce5891a3d3664e80b4f6d23e17d8bbc5d8
ghcr.io/jellyfishsdk/ocean-api:c893dfce5891a3d3664e80b4f6d23e17d8bbc5d8
ghcr.io/jellyfishsdk/playground-api:c893dfce5891a3d3664e80b4f6d23e17d8bbc5d8
ghcr.io/jellyfishsdk/status-api:c893dfce5891a3d3664e80b4f6d23e17d8bbc5d8
ghcr.io/jellyfishsdk/whale-api:c893dfce5891a3d3664e80b4f6d23e17d8bbc5d8

kyleleow · 2023-01-05T05:15:53Z

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

-    return await this.client.call('listgovproposalvotes', [proposalId, masternode, cycle], 'number')
+  async listGovProposalVotes (
+    options: ListGovProposalVotesOptions,
+    pagination: ListGovProposalVotesPagination = {


Correct me if i am wrong, from the code in ain https://github.com/DeFiCh/ain/blob/90c05a00d40381bf6da1b5db05a5e6301cd95a33/src/masternodes/rpc_proposals.cpp#L604

should pagination object be nested inside options?

No, the pagination object is a top level argument, there is no options argument.

In the RPC, all of the arguments can be passed as JSON object, which prevents having to define optional arguments. The PR description talks about this.

ah okay got it, thanks for explaining!

Misunderstood what you meant, we are using the JSON object here to pass all the arguments so the pagination object should be inside the options object. Fixed this in 028f92c.

Thanks for bringing this up🫡

Signed-off-by: Shoham Chakraborty <shhmchk@gmail.com>

delphk

will be good to add test cases for pagination as well

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

delphk · 2023-01-09T03:29:56Z

packages/jellyfish-api-core/__tests__/category/governance/listGovProposalVotes.test.ts

+    const votes1 = await testing.rpc.governance.listGovProposalVotes({
+      proposalId: proposalId,
+      pagination: {
+        start: 0,
+        including_start: true,
+        limit: 2
+      }
+    })
+    const votes2 = await testing.rpc.governance.listGovProposalVotes({
+      proposalId: proposalId,
+      pagination: {
+        start: 2,
+        including_start: true,
+        limit: 2
+      }
+    })


apart from this test, would be also good to test the different combinations of pagination options (eg. start is omitted, including_start is omitted or false, limit is omitted or 0)

Fixed: 09c483e

The test should be in separated it()s describing the pagination condition.

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

fuxingloh · 2023-01-10T09:14:58Z

packages/jellyfish-api-core/__tests__/category/governance/listGovProposalVotes.test.ts

+    const votes1 = await testing.rpc.governance.listGovProposalVotes({
+      proposalId: proposalId,
+      pagination: {
+        start: 0,
+        including_start: true,
+        limit: 2
+      }
+    })
+    const votes2 = await testing.rpc.governance.listGovProposalVotes({
+      proposalId: proposalId,
+      pagination: {
+        start: 2,
+        including_start: true,
+        limit: 2
+      }
+    })


The test should be in separated it()s describing the pagination condition.

Update listGovProposalVotes arguments

73e9f0a

github-actions bot added apps/whale-api area/apps area/packages kind/fix Fix a bug labels Dec 7, 2022

Fix test

38233d2

kyleleow reviewed Jan 5, 2023

View reviewed changes

Bump container to 3.2.1

c825e2a

shohamc1 marked this pull request as ready for review January 5, 2023 13:15

shohamc1 requested review from fuxingloh, canonbrother, ivan-zynesis, kodemon and eli-lim as code owners January 5, 2023 13:15

Merge branch 'main' into shohamc1/update-listgovproposalvotes

5f8f289

Signed-off-by: Shoham Chakraborty <shhmchk@gmail.com>

delphk reviewed Jan 6, 2023

View reviewed changes

shohamc1 added 2 commits January 6, 2023 16:20

Add pagination test

028f92c

Update docs

9b9f312

github-actions bot added the apps/website label Jan 6, 2023

Merge branch 'main' into shohamc1/update-listgovproposalvotes

56c7247

github-actions bot removed area/apps apps/whale-api labels Jan 6, 2023

delphk reviewed Jan 9, 2023

View reviewed changes

packages/jellyfish-api-core/src/category/governance.ts Outdated Show resolved Hide resolved

delphk reviewed Jan 9, 2023

View reviewed changes

fuxingloh requested changes Jan 10, 2023

View reviewed changes

packages/jellyfish-api-core/src/category/governance.ts Outdated Show resolved Hide resolved

Add more pagination tests, fix JSDoc

09c483e

Assert length when testing pagination

44ff246

delphk previously approved these changes Jan 10, 2023

View reviewed changes

fuxingloh requested changes Jan 10, 2023

View reviewed changes

Split tests into smaller units

5c9ac5a

shohamc1 dismissed delphk’s stale review via 5c9ac5a January 10, 2023 09:56

fuxingloh approved these changes Jan 11, 2023

View reviewed changes

fuxingloh merged commit 5abb393 into main Jan 11, 2023

fuxingloh deleted the shohamc1/update-listgovproposalvotes branch January 11, 2023 01:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(jellyfish-api-core): Update listGovProposalVotes arguments #1908

fix(jellyfish-api-core): Update listGovProposalVotes arguments #1908

shohamc1 commented Dec 7, 2022 •

edited

Loading

codeclimate bot commented Dec 7, 2022 •

edited

Loading

netlify bot commented Dec 7, 2022 •

edited

Loading

codecov bot commented Dec 7, 2022 •

edited

Loading

github-actions bot commented Dec 7, 2022 •

edited

Loading

kyleleow Jan 5, 2023 •

edited

Loading

shohamc1 Jan 5, 2023

kyleleow Jan 5, 2023

shohamc1 Jan 6, 2023

delphk left a comment

delphk Jan 9, 2023

shohamc1 Jan 10, 2023

fuxingloh Jan 10, 2023

fuxingloh Jan 10, 2023

fix(jellyfish-api-core): Update listGovProposalVotes arguments #1908

fix(jellyfish-api-core): Update listGovProposalVotes arguments #1908

Conversation

shohamc1 commented Dec 7, 2022 • edited Loading

What this PR does / why we need it:

codeclimate bot commented Dec 7, 2022 • edited Loading

netlify bot commented Dec 7, 2022 • edited Loading

✅ Deploy Preview for jellyfishsdk ready!

codecov bot commented Dec 7, 2022 • edited Loading

Codecov Report

github-actions bot commented Dec 7, 2022 • edited Loading

kyleleow Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

shohamc1 Jan 5, 2023

Choose a reason for hiding this comment

kyleleow Jan 5, 2023

Choose a reason for hiding this comment

shohamc1 Jan 6, 2023

Choose a reason for hiding this comment

delphk left a comment

Choose a reason for hiding this comment

delphk Jan 9, 2023

Choose a reason for hiding this comment

shohamc1 Jan 10, 2023

Choose a reason for hiding this comment

fuxingloh Jan 10, 2023

Choose a reason for hiding this comment

fuxingloh Jan 10, 2023

Choose a reason for hiding this comment

shohamc1 commented Dec 7, 2022 •

edited

Loading

codeclimate bot commented Dec 7, 2022 •

edited

Loading

netlify bot commented Dec 7, 2022 •

edited

Loading

codecov bot commented Dec 7, 2022 •

edited

Loading

github-actions bot commented Dec 7, 2022 •

edited

Loading

kyleleow Jan 5, 2023 •

edited

Loading