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

Replace u64 with Amount type in TxOut #599

Closed
wants to merge 1 commit into from

Conversation

sgeisler
Copy link
Contributor

Some places, most prominently TxOut, were still using u64s for amounts (in sat). This PR replaces these with the Amount newtype and also implements the consensus encode traits for Amount to make it all work.

@sgeisler sgeisler added the API break This PR requires a version bump for the next release label Apr 30, 2021
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

Does it make sense to restrict the Amount type with a MoneyRange check? https://github.com/bitcoin/bitcoin/blob/7fcf53f7b4524572d1d0c9a5fdc388e87eb02416/src/amount.h#L26

This might leak APIwise and will most probably be followed by unwrap() for all use cases. Might also make it awkward to model -1 amount in null TxOut creation.

Side note, also conflicts with #598.

@sgeisler sgeisler force-pushed the 2021-04-use-amount branch 3 times, most recently from 1d2c4ee to 134b056 Compare April 30, 2021 21:03
@sgeisler
Copy link
Contributor Author

@sanket1729 At which level would you want to enforce MoneyRange (I think in rust-bitcoin that would be max_money(…))? On the TxOut level by making value private? Or by introducing some newtype? Or just opportunistically e.g. in consensus decode without really guaranteeing the invariant?

As a side note, I think we represent none-amounts as u64::MAX.

@sgeisler sgeisler force-pushed the 2021-04-use-amount branch 3 times, most recently from b175458 to 3b5f89f Compare April 30, 2021 21:30
dr-orlovsky
dr-orlovsky previously approved these changes May 1, 2021
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

utACK 3b5f89f

I think that Amount type should be definitely improved with MoneyRange-like functionality, since we guarantee at API level that Amount can't exceed consensus value (having internal value made private) - but that's probably better to do in a separate PR.

@dr-orlovsky
Copy link
Collaborator

Needs rebase

Some places, most prominently TxOut, were still using u64s
for amounts (in sat). This PR replaces these with the
Amount newtype and also implements the consensus encode
traits for Amount to make it all work.
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

utACK c193bc7

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

utack c193bc7. Needs rebase

@sanket1729
Copy link
Member

My thinking was similar to what dr-orlovsky suggested. But I think we need a more careful review of bitcoin core codebase to make sure none-amounts don't do random things.

@sanket1729
Copy link
Member

In the interest of taproot release, postponing this again to 0.29 :(

@tcharding
Copy link
Member

Hi @sgeisler, this seems like a change we want, do you have time to work on this still? I have rebased it locally, if you don't have time to work on it feel free to close this and I'll re-open and shepard it in for you (incl. co-authored-by tag).

@apoelstra
Copy link
Member

I believe that @sgeisler has left this space and somebody else will need to create a new version of this PR.

@tcharding
Copy link
Member

Sweet, cheers.

@sgeisler
Copy link
Contributor Author

incl. co-authored-by tag

No need, probably easier to just redo it anyway with all these conflicts.

@sgeisler sgeisler deleted the 2021-04-use-amount branch April 23, 2022 02:50
@Kixunil Kixunil removed this from the 0.29.0 milestone Aug 1, 2022
@Kixunil Kixunil mentioned this pull request Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API break This PR requires a version bump for the next release good first issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants