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.

Sign up for GitHub

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

Jump to bottom

feat(rpc): Implement what we can of getaddresstxids RPC method. #4062

Merged
merged 13 commits into from
Apr 13, 2022
Merged

feat(rpc): Implement what we can of getaddresstxids RPC method. #4062

merged 13 commits into from
Apr 13, 2022

Conversation

oxarbitrage
Copy link
Contributor

Motivation

#3147 is blocked but we decided to do what we can for it and leave the rest of the work for when it gets fully unblocked.

This PR is part of #3147 but it will not close it.

Solution

Implement the method the best we can with what we currently have and return an empty array as response.

Review

I think this is pretty easy to review, most complexity will be in the second part where everything is fully implemented.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

Do the second part of #3147 to close the ticket.

@oxarbitrage oxarbitrage requested review from a team as code owners April 7, 2022 23:38
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team April 7, 2022 23:38
oxarbitrage
oxarbitrage commented Apr 7, 2022
View reviewed changes
Copy link
Contributor Author

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I have some comments.

zebra-chain/src/primitives/zcash_primitives.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
teor2345
teor2345 reviewed Apr 8, 2022
View reviewed 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.

This looks really good!

I'd like to send all the addresses in a single state request, to improve performance.

All my other comments are minor, or there's nothing we can do about them until all the PRs merge.

zebra-chain/src/primitives/zcash_primitives.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
zebra-state/src/service.rs Outdated Show resolved Hide resolved
zebra-state/src/request.rs Outdated Show resolved Hide resolved
zebra-state/src/response.rs Outdated Show resolved Hide resolved
oxarbitrage
oxarbitrage commented Apr 8, 2022
View reviewed changes
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
teor2345
teor2345 previously approved these changes Apr 10, 2022
View reviewed 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.

Looks good!

@mergify mergify bot mentioned this pull request Apr 10, 2022
Closed
19 tasks
mergify bot added a commit that referenced this pull request Apr 10, 2022
@mergify
Merge of #4062
40e41fb
This was referenced Apr 10, 2022
Closed
Closed
mergify bot added a commit that referenced this pull request Apr 10, 2022
@mergify
Merge of #4062
94fd22e
@mergify mergify bot mentioned this pull request Apr 10, 2022
Closed
19 tasks
@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 10, 2022

update

✅ Branch has been successfully updated

@teor2345
Copy link
Contributor

The merge queue's sync past mandatory checkpoint step failed with:

Error: No such container: klt-sync-checkpoint-4079-merge-07dc946-yrzr
https://github.com/ZcashFoundation/zebra/runs/5964224438?check_suite_focus=true#step:9:115

I'm hoping a main merge will merge the necessary fixes correctly. But it could also be a bug on main.

@teor2345
Copy link
Contributor

Error: No such container: klt-sync-checkpoint-4062-merge-394ed8c-ipbo

https://github.com/ZcashFoundation/zebra/runs/5964568674?check_suite_focus=true#step:9:115

mergify bot added a commit that referenced this pull request Apr 11, 2022
@mergify
Merge of #4062
2100906
This was referenced Apr 11, 2022
Closed
Closed
mergify bot added a commit that referenced this pull request Apr 11, 2022
@mergify
Merge of #4062
48a30f6
@mergify mergify bot mentioned this pull request Apr 11, 2022
Closed
19 tasks
@gustavovalverde
Copy link
Member

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Apr 12, 2022

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

This was referenced Apr 12, 2022
Closed
Closed
@teor2345
Copy link
Contributor

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Apr 12, 2022

requeue

☑️ This pull request is already queued

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 12, 2022

update

❌ Base branch update has failed

merge conflict between base and head
err-code: FA479

@teor2345
Copy link
Contributor

Failed due to #3991, let's retry.

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 12, 2022

update

✅ Branch has been successfully updated

@teor2345
Copy link
Contributor

Failed due to #3991, let's retry.

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 13, 2022

update

✅ Branch has been successfully updated

mergify bot added a commit that referenced this pull request Apr 13, 2022
@mergify
Merge of #4062
8b5de68
@mergify mergify bot mentioned this pull request Apr 13, 2022
Closed
40 tasks
@mergify mergify bot merged commit 7b7d22a into main Apr 13, 2022
@mergify mergify bot deleted the issue3147 branch April 13, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

@upbqdn upbqdn Awaiting requested review from upbqdn upbqdn was automatically assigned from ZcashFoundation/cryptographic-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oxarbitrage @teor2345 @gustavovalverde