Replace u64 with Amount type in TxOut#599
Replace u64 with Amount type in TxOut#599sgeisler wants to merge 1 commit intorust-bitcoin:masterfrom
u64 with Amount type in TxOut#599Conversation
c3eb51f to
6174d98
Compare
There was a problem hiding this comment.
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.
1d2c4ee to
134b056
Compare
|
@sanket1729 At which level would you want to enforce As a side note, I think we represent none-amounts as |
b175458 to
3b5f89f
Compare
dr-orlovsky
left a comment
There was a problem hiding this comment.
utACK 3b5f89fdf52c52123dc74d16146ed9b59b6f42a4
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.
|
Needs rebase |
3b5f89f to
bff60ff
Compare
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.
bff60ff to
c193bc7
Compare
sanket1729
left a comment
There was a problem hiding this comment.
utack c193bc7. Needs rebase
|
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. |
|
In the interest of taproot release, postponing this again to 0.29 :( |
|
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. |
|
I believe that @sgeisler has left this space and somebody else will need to create a new version of this PR. |
|
Sweet, cheers. |
No need, probably easier to just redo it anyway with all these conflicts. |
Some places, most prominently
TxOut, were still usingu64s for amounts (in sat). This PR replaces these with theAmountnewtype and also implements the consensus encode traits for Amount to make it all work.