Replace OutputSubstitutionDisabled string context with sub variants#590
Replace OutputSubstitutionDisabled string context with sub variants#590DanGould merged 1 commit intopayjoin:masterfrom
Conversation
Pull Request Test Coverage Report for Build 14041036962Details
💛 - Coveralls |
benalleng
changed the title
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
Great opportunity to add a unit test for each variant here!
payjoin/src/receive/error.rs
Outdated
| OutputValueError, | ||
| OutputScriptPubkeyError, |
There was a problem hiding this comment.
| OutputValueError, | |
| OutputScriptPubkeyError, | |
| DecreasedValue, | |
| ChangedScriptPubkey, |
|
Is there any great advantage to subvariants instead of 2 OutputSelectionError variants? I can't think of any, and this complexity is not insignificant |
benalleng
force-pushed
the
pjos-error-variants
branch
7 times, most recently
from
cf27c96 to
6ad1cbb
Compare
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
The second commit seems unrelated to the goal of this PR so likely belongs in a separate PR for easier/independent review.
payjoin/src/receive/v1/mod.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn test_pjos_disabled_decrease_amount() { |
There was a problem hiding this comment.
I think test_pjos_disabled_decrease_amount, test_pjos_disabled_increase_amount, test_pjos_disabled_script_pubkey could be combined into a single unit test: test_pjos_disabled since it's testing the same functionality just different edge cases. It will avoid some unnecessary repetition setting up the test conditions and keep related logic together.
benalleng
force-pushed
the
pjos-error-variants
branch
3 times, most recently
from
9adcda5 to
9eac27a
Compare
InternalOutputSubstitutionError::OutputSubstitutionDisabled accepts a string context within the error but seeing as there are only two possible error variants here it makes more sense to create separate error contexts to put these errors into.
benalleng
force-pushed
the
pjos-error-variants
branch
from
9eac27a to
6bd53d8
Compare
payjoin/src/receive/v1/mod.rs
Outdated
| use rand::SeedableRng; | ||
|
|
||
| use super::*; | ||
| pub const QUERY_PARAMS: &str = "maxadditionalfeecontribution=182&additionalfeeoutputindex=0"; |
There was a problem hiding this comment.
These are now defined in payjoin-test-utils, but could be added in a follow up
| pub(crate) fn proposal_from_test_vector() -> Result<UncheckedProposal, BoxError> { | ||
| let pairs = url::form_urlencoded::parse(QUERY_PARAMS.as_bytes()); | ||
| let params = Params::from_query_pairs(pairs, &[1])?; | ||
| Ok(UncheckedProposal { psbt: bitcoin::Psbt::from_str(ORIGINAL_PSBT)?, params }) | ||
| } |
There was a problem hiding this comment.
It may be appropriate to host this entire function in payjoin-test-utils, but this could be done with your additional tests
There was a problem hiding this comment.
I will give it a try in that upcoming PR 👍
4 participants
InternalOutputSubstitutionError::OutputSubstitutionDisabled accepts a string context within the error but seeing as there are only two possible error variants here it makes sense to just create two new
InternalOutputSubstitutionErrorvariants to put these errors into.In the second commit I just created some additional test coverage to catch some untested functions and cases.
closes #588