Implement to and from for PublicKey and NodeId #2180
Conversation
lightning/src/routing/gossip.rs
Outdated
|
|
||
| impl From<NodeId> for PublicKey { | ||
| fn from(node_id: NodeId) -> PublicKey { | ||
| PublicKey::from_slice( &node_id.0).unwrap() |
There was a problem hiding this comment.
This isn't synonymous because the unwrap here is reachable. NodeIds were introduced because the pubkey validation is relatively expensive (took several seconds to read a network graph from disk).
There was a problem hiding this comment.
Switched to TryFrom
benthecarman
force-pushed
the
impl-pk-to-node-id
branch
from
9330b07 to
4cf9c81
Compare
Codecov ReportPatch coverage has no change and project coverage change:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2180 +/- ##
==========================================
- Coverage 91.36% 91.35% -0.01%
==========================================
Files 102 102
Lines 50369 50374 +5
Branches 50369 50374 +5
==========================================
+ Hits 46018 46021 +3
- Misses 4351 4353 +2
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
benthecarman
force-pushed
the
impl-pk-to-node-id
branch
from
4cf9c81 to
d6622c0
Compare
lightning/src/routing/gossip.rs
Outdated
| impl FromStr for NodeId { | ||
| type Err = secp256k1::Error; | ||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| let pubkey = PublicKey::from_str(s)?; |
There was a problem hiding this comment.
This is pretty ineffecient, can we just do the hex conversion directly?
There was a problem hiding this comment.
We don't currently have a non-test hex dependency, and it seems kinda wasteful to add one :/
There was a problem hiding this comment.
How should we do this conversion then? I think bitcoin_hashes has one too
There was a problem hiding this comment.
bitcoin_hashes just implements it itself, its like 5 LoC to just walk the chars and to_digit(16) them.
There was a problem hiding this comment.
Idk what I missed the first time around but looks like we did have bitcoin_hashes' version available so i used that
benthecarman
force-pushed
the
impl-pk-to-node-id
branch
from
d6622c0 to
37cd606
Compare
benthecarman
force-pushed
the
impl-pk-to-node-id
branch
2 times, most recently
from
6ed9f88 to
ba7119b
Compare
benthecarman
force-pushed
the
impl-pk-to-node-id
branch
from
ba7119b to
7a37e2c
Compare
These types should be pretty much synonymous