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(rpc): Updated quorum listextended RPC #5104

Merged

Conversation

ogabrielides
Copy link
Collaborator

@ogabrielides ogabrielides commented Dec 12, 2022

Issue being fixed or feature implemented

  • At the request of Platform team, this RPC should accept height instead of count. RPC replies with the signing quorums active at requested height. If height isn't specified, then tip is used for the construction of the reply.
  • Corrections were made on the description of reply model.

What was done?

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

@PastaPastaPasta
Copy link
Member

This is a breaking change, no?

@ogabrielides
Copy link
Collaborator Author

@PastaPastaPasta Technically no because quorum listextended was never released :)

thephez
thephez previously approved these changes Dec 13, 2022
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.

Thanks for updating the help 👍

@UdjinM6
Copy link

UdjinM6 commented Dec 13, 2022

hmm.. can't we have both count and height?

@ogabrielides
Copy link
Collaborator Author

hmm.. can't we have both count and height?

  1. Platform always want signing count
  2. count and height are both optional integer. It would be confusing no?

@UdjinM6
Copy link

UdjinM6 commented Dec 13, 2022

  1. count and height are both optional integer. It would be confusing no?

good point

@PastaPastaPasta
Copy link
Member

Should also add/edit release notes

@ogabrielides
Copy link
Collaborator Author

Should also add/edit release notes

Updated already existing one

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.

utACK

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

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 PastaPastaPasta merged commit fcc8caf into dashpay:develop Dec 14, 2022
@ogabrielides ogabrielides deleted the qr_listextended_argument_block branch December 14, 2022 16:50
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. -->

- At the request of Platform team, this RPC should accept `height`
instead of `count`. RPC replies with the signing quorums active at
requested `height`. If `height` isn't specified, then tip is used for
the construction of the reply.
- Corrections were made on the description of reply model.

## What was done?
<!--- Describe your changes in detail -->


## 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
- [ ] 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
@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