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

Clarify indexes in new_ordered_outputs #2510

Merged
merged 2 commits into from
Jul 21, 2021
Merged

Clarify indexes in new_ordered_outputs #2510

merged 2 commits into from
Jul 21, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 20, 2021 •
edited
Loading

Motivation

In an earlier PR for #2231, I created a new_ordered_outputs function.

But some of its variable names are confusing, and its code ignores an overflow. (Which is currently impossible anyway.)

Solution

  • rename a variable to avoid confusion
  • panic on an impossible overflow (just in case the consensus rules change in future)

This PR is part of #2231, but it does not close that ticket.

Review

@oxarbitrage can review this PR, it isn't blocking anything else.

There are existing tests for this code, and more will be added as part of #2231.

Reviewer Checklist

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

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-Medium labels Jul 20, 2021
@teor2345 teor2345 requested a review from oxarbitrage July 20, 2021 04:45
@teor2345 teor2345 self-assigned this Jul 20, 2021
@mpguerra
Copy link
Contributor

This PR does not close #2231.

I think this line has automatically linked #2231 to this PR...

Screenshot 2021-07-20 at 15 41 24

@teor2345
Copy link
Contributor Author

This PR does not close #2231.

I think this line has automatically linked #2231 to this PR...

Screenshot 2021-07-20 at 15 41 24

Thanks, I fixed it!

@teor2345 teor2345 merged commit 6df17ff into main Jul 21, 2021
@teor2345 teor2345 deleted the clarify-new-outputs branch July 21, 2021 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxarbitrage oxarbitrage oxarbitrage approved these changes

Assignees

@teor2345 teor2345

Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup
Projects
None yet
Milestone
No milestone
Development

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