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

RPC pending - sort by amount #1748

Merged
merged 2 commits into from Mar 10, 2019

Conversation

@guilhermelawless
Copy link
Contributor

commented Feb 19, 2019

Per request of @bbedward . This PR serves to add a sorting option to RPC "pending". It helps services which will now only need to get the first X blocks ordered by amount, to prioritize receiving the most important blocks instead of dust.

I decided to use ptree::sort() due to how cleaner it makes the code vs. having two branches as in ledger ().

===== Wiki entry update =====

Optional "sorting"

Boolean, false by default. Additionally sorts the blocks by their amounts in descending order.

Add 'sorting' option to 'pending' RPC. Change response to include the…
… amount even if threshold is not provided.

@guilhermelawless guilhermelawless changed the title RPC pending - sort by amount WIP: RPC pending - sort by amount Feb 19, 2019

@guilhermelawless guilhermelawless changed the title WIP: RPC pending - sort by amount [WIP] RPC pending - sort by amount Feb 19, 2019

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

To note: can still add the "sorting" option without breaking the RPC, simply by maintaining the same behavior when "threshold" is not given.

As such, this PR could be split in 2: one that adds the sorting option, and one that breaks the RPC to increase consistency. Let me know if this would be the preferred way to go about it.

@zhyatt zhyatt requested a review from SergiySW Feb 26, 2019

@zhyatt zhyatt added the enhancement label Feb 26, 2019

@zhyatt zhyatt added this to the V19.0 milestone Feb 26, 2019

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 27, 2019

Not sure if we want to break existing rpc. It may be confusing, but all libraries rework... Also accounts_pending & wallet pending will return only hashes, even more confusing
Probably we can add "amount" bool field

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Makes sense @SergiySW , i think i achieved something more sensible now.

  • The simple case is kept, to not break RPC.
  • If only "sorting" and/or "threshold" is given, then the response is "hash": "amount" , sorted if using that option.
  • If min_version or source given, the complex version is returned (with different fields)

In summary, RPC not broken and "sorting" added. Also added tests.

@SergiySW
Copy link
Collaborator

left a comment

LGTM
Please, make message with updated wiki entry

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Done. Btw, the wiki is missing the optional "min_version"

@guilhermelawless guilhermelawless changed the title [WIP] RPC pending - sort by amount RPC pending - sort by amount Feb 27, 2019

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Label should change to rpc non breaking.

@zhyatt zhyatt added this to CP1 in V19 Feb 28, 2019

@SergiySW SergiySW merged commit 02db8a6 into nanocurrency:master Mar 10, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@guilhermelawless guilhermelawless deleted the guilhermelawless:rpc/pending_sorting branch Mar 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.