Refactor RecvPacket handler and Acknowledgement types
#364
Conversation
Codecov ReportBase: 60.00% // Head: 60.42% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #364 +/- ##
==========================================
+ Coverage 60.00% 60.42% +0.42%
==========================================
Files 124 124
Lines 15650 15665 +15
==========================================
+ Hits 9391 9466 +75
+ Misses 6259 6199 -60
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 at Codecov. |
| } | ||
|
|
||
| impl AsRef<[u8]> for Acknowledgement { | ||
| fn as_ref(&self) -> &[u8] { | ||
| match self { | ||
| Acknowledgement::Success(_) => ACK_SUCCESS_B64.as_bytes(), | ||
| Acknowledgement::Success(_) => r#"{"result":"AQ=="}"#.as_bytes(), |
There was a problem hiding this comment.
Note that I'm still investigating whether this is correct
There was a problem hiding this comment.
If it is indeed correct, can we use the serde_json implementation instead of the hardcoded version? This would help ensure the two don't go out of sync and is a bit cleaner imho than hardcoding some JSON in there, at a small cost of efficiency though.
There was a problem hiding this comment.
What I still need to investigate is whether spaces matter in the resulting json. In the case where they do, I would prefer the hardcoded version since I don't know if serde_json::to_string() adds spaces or not, and I don't want to have to go check every time I forget (or be worried every time we update serde-json). WDYT?
There was a problem hiding this comment.
Note that I confirmed that this is the correct output.
There was a problem hiding this comment.
What I still need to investigate is whether spaces matter in the resulting json. In the case where they do, I would prefer the hardcoded version since I don't know if serde_json::to_string() adds spaces or not, and I don't want to have to go check every time I forget (or be worried every time we update serde-json). WDYT?
Spaces don't matter; see issue comment. However, what I don't like about serde_json::to_string() is that it returns a Result which we would have to unwrap(). Ideally, I would prefer if we didn't have any unwrap()s in the code, as they make audits of the code more difficult (that's an extra thing to check), and every time I update serde-json I'd ideally need to verify that the Error conditions didn't change. What's your take on this?
There was a problem hiding this comment.
Will merge for now; we can always open an issue to improve this
|
Note that I fixed the Otherwise, everything is in there, and is ready for a final review. |
* refactor * change `AsRef` impl for ICS-20 Ack * test success ack * revert `Acknowledgement::AsRef` for `Error` * document standard divergence * Rename GenericAcknowledgement * refactor transfer on_recv_packet() * Remove `Default` for acks * Rename TokenTransferAcknowledgement * changelog * Fix error ack for ics20 * ics20: test error ack * use is_successful() * refactor test * remove AsRef
Closes: #361
Closes: #369
Description
This PR contains consensus-breaking changes: serialization of successful acknowledgements of packets in
onRecvPacket()changed (in order to become compliant with ibc-go).PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.