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

feat: Enrich quorum listextended rpc reply #5114

Conversation

ogabrielides
Copy link
Collaborator

@ogabrielides ogabrielides commented Dec 21, 2022

Issue being fixed or feature implemented

What was done?

Added the fields numValidMembers and healthRatio in quorum listextended RPC reply, as we need a quick way to see the health of all quorums with a single command.
healthRatio range is [0.0 - 1.0]
Note: The decision to include both fields was taken because we need cover the case where a quorum was created with minSize members but all of them were valid.

How Has This Been Tested?

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@ogabrielides
Copy link
Collaborator Author

@PastaPastaPasta Again, this is not a breaking change since this RPC was never released yet.

Copy link
Collaborator

@thephez thephez left a comment

Choose a reason for hiding this comment

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

A couple small suggestions on wording.

Copy link
Collaborator

@thephez thephez left a comment

Choose a reason for hiding this comment

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

Slightly-tested ACK

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for squash merge

@PastaPastaPasta
Copy link
Member

@UdjinM6 please post-merge review this

@PastaPastaPasta PastaPastaPasta merged commit 07b5a45 into dashpay:develop Dec 23, 2022
@ogabrielides ogabrielides deleted the quorum_listextended_badmembers_info branch December 24, 2022 09:22
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 24, 2022
<!--
*** Please remove the following help text before submitting: ***

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it
improves
Dash Core user experience or Dash Core developer experience
significantly:

* Any test improvements or new tests that improve coverage are always
welcome.
* All other changes should have accompanying unit tests (see
`src/test/`) or
functional tests (see `test/`). Contributors should note which tests
cover
modified code. If no tests exist for a region of modified code, new
tests
  should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or
an
explanation of the potential issue as well as reasoning for the way the
bug
  was fixed.
* Features are welcome, but might be rejected due to design or scope
issues.
If a feature is based on a lot of dependencies, contributors should
first
  consider building the system outside of Dash Core, if possible.
-->

## Issue being fixed or feature implemented
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->


## What was done?
<!--- Describe your changes in detail -->
Added the fields `numValidMembers` and `healthRatio` in `quorum
listextended` RPC reply, as we need a quick way to see the health of all
quorums with a single command.
`healthRatio` range is `[0.0 - 1.0]`
Note: The decision to include both fields was taken because we need
cover the case where a quorum was created with `minSize` members but all
of them were valid.

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->


## Breaking Changes
<!--- Please describe any breaking changes your code introduces -->


## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation

**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

post-merge utACK

@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants