Show JIT channel fee in wallet history #1103
Conversation
Restioson
force-pushed
the
show-jit-fee-in-history
branch
from
5593647 to
077d9cf
Compare
|
@Restioson That was not the intention of that ticket 🙈. I suggested a similar display as for the matching fees 😅
This change goes more into the direction of showing transaction details. #872 Did you maybe linked the wrong ticket? |
crates/orderbook-commons/src/lib.rs
Outdated
| @@ -21,7 +21,11 @@ pub use crate::price::Price; | |||
| pub use crate::price::Prices; | |||
|
|
|||
| /// The prefix used in the description field of an order-matching fee invoice to be paid by a taker. | |||
| pub const FEE_INVOICE_DESCRIPTION_PREFIX_TAKER: &str = "taker-fee-"; | |||
| pub const FEE_INVOICE_DESCRIPTION_PREFIX_TAKER: &str = "taker-fee"; | |||
There was a problem hiding this comment.
📣 I appreciate that it's probably nicer to remove this dash, but I think this is a breaking change. Probably worth avoiding that headache.
As a side note, I am pretty sure the prefix idea that I introduced is not very good and there were better alternatives 🙈
There was a problem hiding this comment.
📣 I appreciate that it's probably nicer to remove this dash, but I think this is a breaking change. Probably worth avoiding that headache.
Yea, most definitely 😅
Maybe for now I should add a legacy match to fallback to the old prefix with the hyphens.
The reason I added this was because there isn't really a way to match on multiple prefixes cleanly, whereas if you define the syntax for a structured payment invoice description as
{tag} {additional_data}
then you can just split_once by space and match on the tag to get what kind of invoice it is.
However, it's worth noting that both this implementation and the previous implementation could lead to some strange results.
For instance, what would happen if an invoice were paid by a mischievous (or even malicious) user with the description taker-fee-NotAnOrderId?!/.sidfj? At best this would display wrong, worse it could crash the app, and at worst it could introduce a security vulnerability.
It's probably worth workshopping how this should work 😅
|
I've pushed a commit to revert, and happy to squash up once we've resolved the discussion. Please don't merge until then |
Hmm.. on main it's now simply shown as an outgoing payment instead of showing directly in the title that it is a JIT channel open fee payment. It seems inconsistent that we show a matching fee in the title but not for the JIT channel opening fee. |
|
I might not be understanding you correctly here so bear with me as I repeat what I understand from what you said 😅
Agree, that's what this PR is changing.
On this branch, it does show as channel open fee. Let me show another screenshot in case it wasn't clear:
|
Oh that's great! I just saw the other screenshot and thought this was the only change 😅 Well done 👍 looks nice. Sorry for the confusion 😅 |
|
No problem! So I guess it's just the query about prefixes vs splitting to resolve? Maybe that should go in a follow-up and just stick to prefixes for now, though. In that case I can rebase this and squash it up before merge |
Restioson
force-pushed
the
show-jit-fee-in-history
branch
from
4d7c001 to
550c039
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Fixes #982