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

Move Utxo type to zebra-chain #2481

Merged
merged 1 commit into from
Jul 12, 2021

Conversation

oxarbitrage
Copy link
Contributor

Motivation

In the value pools design we created several methods that receive a Hashmap with Utxos, just as:

However, the Utxo is a zebra-state type.

Note: zebra-chain do not depend on zebra-state

Designs

Value Pools Design

Solution

Move the state Utxo type into zebra-chain::transparent so we can have it available in method signatures. This is a pretty noisy change so i separated it from the value pool implementation but it is part of #1895

Review

I am not sure if we have a better alternative but i think this is needed. Anyone can review the code.

Reviewer Checklist

  • Current tests pass all.

Follow Up Work

Continue the implementation of value pools in #1895

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-Medium labels Jul 12, 2021
Copy link
Collaborator

@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 this change is low-risk - we have existing Utxo tests, and the Rust compiler checks imports.

Thanks for splitting it out into a separate PR!

There are alternatives, but they make the code less readable and harder to test. For example, we could:

  • impl extra methods on Block in zebra_state - but then those methods aren't available unless a crate depends on zebra_state
  • add the method in zebra_chain, and make it generic over a trait - but that duplicates the method signatures

@teor2345
Copy link
Collaborator

I'm going to merge this now, because splitting #2420 also depends on UTXOs.

@teor2345 teor2345 merged commit f7026d7 into ZcashFoundation:main Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants