Impl PartialOrd and Ord for Invoice

[benthecarman]

No description provided.

[TheBlueMatt]

Hmm, I'd kinda rather we just derive(PartialOrd, Ord) on ~everything in the crate (as required) over allocating a string just to do a comparison?

[codecov-commenter]

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.59 ⚠️

Comparison is base (818dbdf) 91.50% compared to head (2ddce64) 90.91%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2279      +/-   ##
==========================================
- Coverage   91.50%   90.91%   -0.59%     
==========================================
  Files         104      104              
  Lines       52087    52770     +683     
  Branches    52087    52770     +683     
==========================================
+ Hits        47660    47976     +316     
- Misses       4427     4794     +367     

Impacted Files	Coverage Δ
lightning/src/ln/features.rs	96.08% <0.00%> (-2.38%)	⬇️
lightning-invoice/src/lib.rs	79.22% <70.83%> (-0.81%)	⬇️
lightning/src/ln/mod.rs	95.00% <100.00%> (+8.63%)	⬆️
lightning/src/routing/gossip.rs	89.77% <100.00%> (ø)
lightning/src/routing/router.rs	94.43% <100.00%> (-0.67%)	⬇️

... and 18 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[benthecarman]

Hmm, I'd kinda rather we just derive(PartialOrd, Ord) on ~everything in the crate (as required) over allocating a string just to do a comparison?

Tried doing that, it boils down to not having it impl'd on RecoverableSignature which is part of secp

[wpaulino]

Tried doing that, it boils down to not having it impl'd on RecoverableSignature which is part of secp

rust-bitcoin should support it then. Signature already has them derived so there shouldn't be a problem.

[benthecarman]

rust-bitcoin should support it then. Signature already has them derived so there shouldn't be a problem.

Maybe I am an idiot and don't see it but I don't think it exists in rust-bitcoin. This is the only use of RecoverableSignature in rust-bitcoin:

https://github.com/search?q=repo%3Arust-bitcoin%2Frust-bitcoin%20RecoverableSignature&type=code

I made a PR in rust-secp256k1 for now: rust-bitcoin/rust-secp256k1#611

Unless do you mean I do something like this?

impl PartialOrd for InvoiceSignature {
	fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
		self.0.serialize_compact().1.partial_cmp(&other.0.serialize_compact().1)
	}
}

impl Ord for InvoiceSignature {
	fn cmp(&self, other: &Self) -> Ordering {
		self.0.serialize_compact().1.cmp(&other.0.serialize_compact().1)
	}
}

[TheBlueMatt]

Yea, rust-secp is the right place for that fix, sorry for the confusion. This LGTM as-is and we should clean it up after rust-secp.