smite: Implement update_fail_malformed_htlc codec#38
Conversation
|
hey @Chand-ra some nit pick comments from my side otherwise your PR LGTM |
|
Thanks @Vineet1101, but I cannot see your comments. |
|
Also please make sure CI passes before requesting the next review. You should be able to run all the necessary commands locally. |
Chand-ra
force-pushed
the
fail_malformed
branch
2 times, most recently
from
66dc022 to
da5bf58
Compare
|
Rebased on top of the latest master to resolve a merge conflict. |
ekzyis
left a comment
ekzyis
left a comment
There was a problem hiding this comment.
LGTM, just a few nits, but we could also fix not using the constants everywhere in one PR, like here and new cases where we could use SHA256_HASH_SIZE
| msg_type::UPDATE_FAIL_MALFORMED_HTLC | ||
| ); | ||
| assert_eq!( | ||
| Message::GossipTimestampFilter(GossipTimestampFilter::no_gossip([0u8; 32])).msg_type(), |
There was a problem hiding this comment.
huh looks like I didn't use CHAIN_HASH_SIZE myself here
There was a problem hiding this comment.
Maybe it's worth it to replace usage of CHAIN_HASH_SIZE with SHA256_HASH_SIZE, since they're essentially the same thing?
There was a problem hiding this comment.
I was thinking about that too. But wouldn't this also apply to TXID_SIZE and CHANNEL_ID_SIZE? A txid is also just a sha256 hash, and a channel id is a txid XOR output index.
Chand-ra
force-pushed
the
fail_malformed
branch
from
da5bf58 to
68ef5ec
Compare
I think it makes sense to at least use the constant in the new code we're introducing with this PR. As for the other instances, I agree, it makes more sense to fix them in follow-up PR. |
Chand-ra
force-pushed
the
fail_malformed
branch
2 times, most recently
from
cdabb3e to
c5a49b5
Compare
Chand-ra
force-pushed
the
fail_malformed
branch
from
c5a49b5 to
27192f2
Compare
4 participants
PRs #27 and #33 cover adding, fulfilling, and failing HTLCs during commitment transaction updates, but the specific "malformed" case is missing. Add the codec for it.