Fix TxOut::is_fee, add confidential is_explicit and unwrap_explicit, add Transaction::fee

[stevenroose]

No description provided.

[jonasnick]

would be easy to add a test for this. Otherwise ACK

[RCasatta]

o.minimum_value() is used to get the Explicit Value, which is always the case for fee, so it works but it was unclear to me by looking at this function.

[stevenroose]

@RCasatta I agree, I had to check it as well. This is how it was done in our integration tests, I ported the method here. I guess since we use is_fee, we can assert the value to be explicit afterwards. Let me clean that up and add a test.

[stevenroose]

I noticed that is_fee was actually incomplete according to Elements Core. So I fixed that and added is_explicit and unwrap_explicit methods to the confidential types to help.

[RCasatta]

did you consider having unwrap_explicit() returning a Result? This way you can remove the panic! and also remove is_explicit() and use unwrap_explicit().is_ok() instead

[stevenroose]

Yeah but then it's not really an unwrap method like in the stdlib. We could have fn explicit(&self) -> Option<T> instead, like Result::ok; in that case our usage would be value.explicit().unwrap(), which for me is fine as well. In that case, the is_explicit method could just return .explicit().is_some().

[RCasatta]

yeah, Option or like it is now is ok

[stevenroose]

I changed it to a Option, can you re-review and ack? I'd like to have this to add feerates to hal-elements.

[RCasatta]

ack cbb9674

[stevenroose]

@apoelstra could you perhaps give me owner permissions on this repo so that I can merge PRs that are ACKed, but not "github-approved"? Currently I can't. I'll try self-approve, but that's weird.

[stevenroose]

update: you can't self-approve. Bravo, GitHub :)

[jonasnick]

ACK cbb9674

[shesek]

Is there a reason explicit() was added to Value but not to Asset or Nonce, while unwrap_explicit() was not added to Value but only to the others?

I was refactoring some of my code to take advantage of the new methods, was kind of surprised to see that Asset::explicit() does not exists. I think it would make sense to have all three methods on all three structs.

[shesek]

Nice, @stevenroose unified them here: 5d826e8