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

trivial: add missing rpc help messages, remove segwit references, dashify help text, undashify code comments #5852

Merged
merged 8 commits into from
Feb 9, 2024

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Jan 31, 2024

Issue being fixed or feature implemented

This pull request is a follow-up to some feedback received on dash#5834 as the patterns highlighted were present in different parts of the codebase and hence not corrected within the PR itself but addressed separately.

This is that separate PR 🙂 (with some additional cleanup of my own)

What was done?

  • This pull request will remain a draft until dash#5834 as it will introduce more changes that will need to be corrected in this PR.
  • Code introduced that is unique to Dash Core (CoinJoin, InstantSend, etc.) has been excluded from un-Dashification as the purpose of it is to reduce backport conflicts, which don't apply in those cases.
    • CWallet::CreateTransaction and the CreateTransactionTest fixture have been excluded as the former originates from dash#3668 and the latter from dash#3667 and are distinct enough to be unique to Dash Core.
  • There are certain Dashifications and SegWit-removals that prove frustrating as it would break compatibility with programs that rely on the naming of certain keys
    • getrawmempool, getmempoolancestors, getmempooldescendants and getmempoolentry return vsize which is currently an alias of size. I have been advised to retain vsize in lieu of potential future developments. (this was originally remedied in 219a1d0 but has since been dropped)
    • getaddressmempool, getaddressutxos and getaddressdeltas all return a value with the key satoshis. This is frustrating to rename to duffs for compatibility reasons.
    • decodepsbt returns (if applicable) non_witness_utxo which is frustrating to rename simply to utxo for the same reason.
    • analyzepsbt returns (if applicable) estimated_vsize which frustrating to rename to estimated_size for the same reason.

How Has This Been Tested?

Breaking Changes

None

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
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

Copy link

github-actions bot commented Feb 1, 2024

This pull request has conflicts, please rebase.

@kwvg kwvg marked this pull request as ready for review February 8, 2024 16:25
@knst knst modified the milestones: 21, 20.1 Feb 8, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK

Seems as there's no breaking changes in this PR.

Btw, PR description mentions some possible changes such as rename "non_witness_utxo" which annoys me too - let's prepare one more PR for v21?

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. No breaking changes detected. @thephez please confirm style is correct

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.

Looks okay to me. Just one nit, but nothing that prevents merging.

@@ -369,7 +369,7 @@ static UniValue gettxchainlocks(const JSONRPCRequest& request)
{RPCResult::Type::OBJ, "", "",
{
{RPCResult::Type::NUM, "height", "The block height"},
{RPCResult::Type::BOOL, "chainlock", "Chainlock status for the block containing the transaction"},
{RPCResult::Type::BOOL, "chainlock", "The state of the corresponding block ChainLock"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: imo the original wording was easier to understand

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The phrase "The state of the corresponding block ChainLock" was used more and seemed more concise.

@PastaPastaPasta PastaPastaPasta merged commit d9c549e into dashpay:develop Feb 9, 2024
7 of 14 checks passed
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.

5 participants