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

refactor: address comments from #3415: Prepare for changes in ZIP-244 #3446

Merged
merged 3 commits into from
Feb 1, 2022

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Jan 31, 2022

Motivation

#3415 was accidentally merged without addressing review comments. This PR addresses them.

Specifications

Designs

Solution

Review

Reviewer Checklist

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

Follow Up Work

@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #3446 (bc1ac4f) into main (499ae89) will decrease coverage by 0.18%.
The diff coverage is 79.34%.

@@            Coverage Diff             @@
##             main    #3446      +/-   ##
==========================================
- Coverage   78.34%   78.15%   -0.19%     
==========================================
  Files         267      272       +5     
  Lines       31526    31532       +6     
==========================================
- Hits        24698    24644      -54     
- Misses       6828     6888      +60     

Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Consider this approved. I'm not marking it as approved in case it gets merged again 😅

I left two optional suggestions, but feel free to merge without them :)

Comment on lines +483 to +484
} else {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit: this feels redundant to me 🤔

Suggested change
} else {
continue;

Copy link
Contributor

Choose a reason for hiding this comment

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

I kinda appreciate the explicit continue in the loop 😅

zebra-consensus/src/transaction.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 changed the title refactor: address comments from #3415 refactor: address comments from #3415: Prepare for changes in ZIP-244 Jan 31, 2022
Co-authored-by: Janito Vaqueiro Ferreira Filho <[email protected]>
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

LGTM!

Let's see if Mergify will merge with at least one unresolved conversation 👀

@mergify mergify bot merged commit 494b7dc into main Feb 1, 2022
@mergify mergify bot deleted the zcash-script-v5-tx branch February 1, 2022 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants