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): Implement the z_validateaddress RPC #6185

Merged
merged 14 commits into from
Feb 20, 2023
Merged

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Feb 17, 2023

Motivation

Close #6083.

Specifications

https://zcash.github.io/rpc/z_validateaddress.html

Solution

This PR implements the following fields:

  • isvalid,
  • address,
  • address_type,
  • is_mine.

To implement UAs, I used the zcash_address crate.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@upbqdn upbqdn requested review from a team as code owners February 17, 2023 00:02
@upbqdn upbqdn requested review from dconnolly and removed request for a team February 17, 2023 00:03
@github-actions github-actions bot added the C-feature Category: New features label Feb 17, 2023
@upbqdn upbqdn self-assigned this Feb 17, 2023
@upbqdn upbqdn added A-rust Area: Updates to Rust code P-High 🔥 A-rpc Area: Remote Procedure Call interfaces labels Feb 17, 2023
@upbqdn upbqdn marked this pull request as draft February 17, 2023 00:05
@upbqdn
Copy link
Member Author

upbqdn commented Feb 17, 2023

An initial basic test shows that the output is identical to zcashd.
Request:
z_validateaddress t2UNzUUx8mWBCRYPRezvA363EYXyEpHokyi

Querying zebrad test chain at height >=2232411...

real    0m0.002s
user    0m0.002s
sys     0m0.000s

Querying zcashd test chain at height >=2232411...

real    0m0.002s
user    0m0.002s
sys     0m0.000s


Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.ESy1lXAX0v.rpc-diff/zebrad-test-2232411-z_validateaddress.json:
{
  "isvalid": true,
  "address": "t2UNzUUx8mWBCRYPRezvA363EYXyEpHokyi",
  "address_type": "p2sh",
  "ismine": false
}

teor2345
teor2345 previously approved these changes Feb 17, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for the address API changes.

We're just missing the snapshot files, so this PR will fail CI until they are added.

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Merging #6185 (be14b2d) into main (31382d2) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6185      +/-   ##
==========================================
+ Coverage   77.98%   77.99%   +0.01%     
==========================================
  Files         304      304              
  Lines       39197    39197              
==========================================
+ Hits        30568    30573       +5     
+ Misses       8629     8624       -5     

@upbqdn upbqdn marked this pull request as ready for review February 20, 2023 13:17
@upbqdn
Copy link
Member Author

upbqdn commented Feb 20, 2023

I implemented all suggestions, semantic UA checks, and I updated the PR description to reflect the new changes.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think we're missing some snapshot cases, but luckily we already have test vectors in z_listunifiedreceivers.

There might also be a minor future compatibility issue.

I don't think these are blockers to going ahead and merging this, so I'd suggest doing them in a follow-up PR.

We can also repeat the zcash-rpc-diff test on that PR.

mergify bot added a commit that referenced this pull request Feb 20, 2023
@mergify mergify bot merged commit 83d038c into main Feb 20, 2023
@mergify mergify bot deleted the 6083-z-validateaddress branch February 20, 2023 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces A-rust Area: Updates to Rust code C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement z_validateaddress RPC
3 participants