Repository navigation
Remove url from payjoin-ffi in favor of native payjoin::url - #1907
Conversation
Coverage Report for CI Build 36450575032Warning No base build found for commit Coverage: 86.869%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
581eef9 to
ee62798
Compare
|
seems like a simple oversight that got forgotten to be removed to me |
ee62798 to
677b701
Compare
I also added some additional url specific tests with the uri unit tests to demonstrate that the url validation is happening as a precondition to the uri validation.
677b701 to
e92e9d3
Compare
DanGould
left a comment
DanGould
left a comment
There was a problem hiding this comment.
Oof. Those tests were pretty broken before. egregious as your pr body says indeed. I approved that test I think, it started as an honest TODO and then that got lifted accidentally. In hindsight I think it's totally minor but.
I wondered iwhy Kotlin was left out, but it was actually already right.
This is a good reason for BIP{,3}21 test cases to be a single data file under payjoin-ffi/. Did decoded values not get checked? Unfinished tests should be written to fail and then ignored like with the #[ignore] procmacro in rust. Spooky but doesn't seem to have any bad consequences, fortunately. Hygiene fix makes me feel clean .
ACK e92e9d3
| public void UrlEncodedPayjoinParameter() | ||
| { | ||
| var uri = "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?amount=1&pj=https://example.com?ciao"; | ||
| var result = Url.Parse(uri); | ||
| var uri = "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?amount=1&pj=https%3A%2F%2Fexample.com%3Fciao"; | ||
| var result = Uri.Parse(uri); |
There was a problem hiding this comment.
Was this test just incorrectly written before?
Closes #1905
This removes the url dep in payjoin-ffi in favor of using the native payjoin url. I feel like there was a reason we told ourselves why we didn't want this at first but I can't find it for the life of me so I thought I should just put it out into the wild to see what we think.
I also notices there were some pretty egregious test errors where we were using url parsing instead of uri parsing.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.