Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

rpc: votegov: Add support for owner and operator address#1717

Merged
Bushstar merged 14 commits intoDeFiCh:masterfrom
DocteurPing:feat/vote_with_address_for_proposal
Feb 6, 2023
Merged

rpc: votegov: Add support for owner and operator address#1717
Bushstar merged 14 commits intoDeFiCh:masterfrom
DocteurPing:feat/vote_with_address_for_proposal

Conversation

Copy link

DocteurPing commented Jan 30, 2023 *
edited
Loading

Summary

Enables support for address support on votegov to vote with an address in addition to the currently accepted masternode ID.

How

  • Over the wire, TX still uses masternode ID.
  • The RPC API now auto selects the masternode ID based on the address provided.
  • The selection order:
    • MasternodeID
    • OwnerAdress
    • OperatorAddress

Limitations

  • Throws an error when there is no reasonable choice available (invalid address/masternodeId, address does not own/operate a masternode).

DocteurPing requested review from Bushstar and Mixa84 as code owners January 30, 2023 12:28
DocteurPing requested a review from a user January 30, 2023 12:28
DocteurPing requested review from Jouzo and prasannavl as code owners January 30, 2023 12:28
defichain-bot added the kind/feature label Jan 30, 2023
prasannavl changed the title feat: add possibility to vote with owner/operator address instead of ... rpc: votegov: Add support for owner and operator address Jan 31, 2023
shohamc1 assigned DocteurPing Jan 31, 2023
prasannavl added the v/3.2.3 label Feb 1, 2023
prasannavl mentioned this pull request Feb 9, 2023
19 tasks
Bushstar reviewed Feb 1, 2023
auto propId = ParseHashV(request.params[0].get_str(), "proposalId");
auto mnId = ParseHashV(request.params[1].get_str(), "masternodeId");
std::string id = request.params[1].get_str();
uint256 mnId = uint256();
Copy link
Contributor

Bushstar Feb 1, 2023

Choose a reason for hiding this comment

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

Note that you only need as uint256 will default initialise itself as the class has a default constructor.

uint256 mnId;

Bushstar reviewed Feb 1, 2023
auto node = view.GetMasternode(mnId);
if (!node) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("The masternode %s does not exist", mnId.ToString()));
if (id.length() == 34) {
Copy link
Contributor

Bushstar Feb 1, 2023

Choose a reason for hiding this comment

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

Address can be 26-35 chars long for P2PKH and 42 for P2WPKH. I'd work on getting the correct mnId before calling GetMasternode.

Bushstar reviewed Feb 1, 2023
if (!node) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("The masternode %s does not exist", mnId.ToString()));
if (id.length() == 34) {
CTxDestination dest = DecodeDestination(id);
Copy link
Contributor

Bushstar Feb 1, 2023

Choose a reason for hiding this comment

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

You need to call IsValidDestination on dest to make sure it is valid. You can do this in the else statement.

if (id.length() == 64) {
mnId = ParseHashV(id, "masternodeId");
} else {
CTxDestination dest = DecodeDestination(id);
if (!IsValidDestination(dest)) {
throw JSONRPCError(RPC_INVALID_PARAMETER,
strprintf("The masternode id or address is not valid: %s", id));
}
// Call GetMasternodeIdByOwner and GetMasternodeIdByOperator here.
}

Bushstar reviewed Feb 1, 2023
const CKeyID ckeyId = dest.index() == PKHashType ? CKeyID(std::get(dest)) : CKeyID(std::get(dest));
auto masterNodeIdByOwner = view.GetMasternodeIdByOwner(ckeyId);
if (!masterNodeIdByOwner) {
auto masterNodeIdByOperator = view.GetMasternodeIdByOperator(ckeyId);
Copy link
Contributor

Bushstar Feb 1, 2023

Choose a reason for hiding this comment

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

Check that masterNodeIdByOperator is valid or you will get a std::bad_optional_access thrown on masterNodeIdByOperator.value(). I'd structure it like the following:

if (auto masterNodeIdByOwner = view.GetMasternodeIdByOwner(ckeyId)) {
mnId = masterNodeIdByOwner.value();
} else if (auto masterNodeIdByOperator = view.GetMasternodeIdByOperator(ckeyId)) {
mnId = masterNodeIdByOperator.value();
}

Bushstar reviewed Feb 2, 2023
throw JSONRPCError(RPC_INVALID_PARAMETER,
strprintf("The masternode id or address is not valid: %s", id));
}
const CKeyID ckeyId = dest.index() == PKHashType ? CKeyID(std::get(dest)) : CKeyID(std::get(dest));
Copy link
Contributor

Bushstar Feb 2, 2023

Choose a reason for hiding this comment

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

We need to make sure that the provided address is either P2PKH or a P2WPKH, otherwise if someone passes a different variant then std::get will throw a std::bad_variant_access.

const CKeyID ckeyId = dest.index() == PKHashType ? CKeyID(std::get(dest))
: dest.index() == WitV0KeyHashType ? CKeyID(std::get(dest))
: CKeyID();

Bushstar previously approved these changes Feb 2, 2023
Mixa84 added v/3.2.4 and removed v/3.2.3 labels Feb 3, 2023
shohamc1 reviewed Feb 6, 2023
throw JSONRPCError(RPC_INVALID_PARAMETER,
strprintf("The masternode id or address is not valid: %s", id));
}
const CKeyID ckeyId = dest.index() == PKHashType ?
Copy link
Contributor

shohamc1 Feb 6, 2023

Choose a reason for hiding this comment

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

A bit too hard to read IMO, better to break it down into ifs.

DocteurPing dismissed Bushstar's stale review via 06e24c2 February 6, 2023 05:35
Bushstar merged commit 3e92abf into DeFiCh:master Feb 6, 2023
DocteurPing deleted the feat/vote_with_address_for_proposal branch February 7, 2023 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Bushstar Bushstar left review comments

Mixa84 Awaiting requested review from Mixa84

prasannavl Awaiting requested review from prasannavl

Jouzo Awaiting requested review from Jouzo

+1 more reviewer

shohamc1 shohamc1 left review comments

Reviewers whose approvals may not affect merge requirements

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants