Impl Writeable and Readable for Address#3206
Impl Writeable and Readable for Address#3206jbesraa wants to merge 1 commit intolightningdevkit:mainfrom
Writeable and Readable for Address#3206Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3206 +/- ##
==========================================
- Coverage 89.80% 89.76% -0.05%
==========================================
Files 122 122
Lines 101621 101650 +29
Branches 101621 101650 +29
==========================================
- Hits 91257 91242 -15
- Misses 7677 7706 +29
- Partials 2687 2702 +15 ☔ View full report in Codecov by Sentry. |
|
Hmmm, I'm not really a fan of implementing the LDK serialization traits for rust-bitcoin types where rust-lightning doesn't have a use for them. How hard is it to have this logic in ldk-node? |
Well, we can't as we can't implement foreign traits on foreign objects? I guess we could create another (or even wrapper type) for it, but |
| fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> { | ||
| let v: WithoutLength<String> = Readable::read(r)?; | ||
| match Address::from_str(&v.0) { | ||
| Ok(addr) => Ok(WithoutLength(addr.assume_checked())), |
There was a problem hiding this comment.
Hmmmm, not sure if we should assume_checked here? Kinda defeats the purpose of the network-checking API, but of course on the flip side there's really not much we could practically do to actually check the network...
| } | ||
| } | ||
|
|
||
| impl Writeable for Address { |
There was a problem hiding this comment.
Is there a need to do both the WithoutLength version and the with?
| (addr_str.len() as u16).write(w)?; | ||
| w.write_all(self.to_string().as_bytes()) |
There was a problem hiding this comment.
Just write a String directly?
| let len = <u16 as Readable>::read(r)? as usize; | ||
| let mut buf = vec![0; len]; | ||
| r.read_exact(&mut buf)?; | ||
| match Address::from_str(&String::from_utf8(buf).map_err(|_| DecodeError::InvalidValue)?) { |
|
As mentioned over at the LDK Node PR, it was discussed that we'll likely use the receiver's |
3 participants
resolves #3205