Export standalone commitment creators for PacketCommitment & AcknowledgementCommitment
#492
Conversation
Farhad-Shabani
added
the
A: breaking
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #492 +/- ##
==========================================
- Coverage 71.52% 71.48% -0.04%
==========================================
Files 125 125
Lines 15890 15858 -32
==========================================
- Hits 11365 11336 -29
+ Misses 4525 4522 -3
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. |
| @@ -69,3 +69,52 @@ impl From<Vec<u8>> for AcknowledgementCommitment { | |||
| Self(bytes) | |||
| } | |||
| } | |||
|
|
|||
| pub use compute::{compute_ack_commitment, compute_packet_commitment}; | |||
There was a problem hiding this comment.
why not just remove the compute module altogether?
There was a problem hiding this comment.
To keep it modular (achieves a different goal than the code above), better-organized import calls.
It's okay if we keep this, I'll also put pub(crate) before mod compute ?
There was a problem hiding this comment.
I don't like that there are now 2 paths to the same items, and both are used internally (e.g from commitment, and from compute).
achieves a different goal than the code above
I think they do achieve the same goal: the commitment module holds structs and free functions related to packet commitments. I think this is also why you think it makes sense to expose it as such to the users ;).
I guess the TLDR of my opinion is that separating free functions from struct definitions is not sufficient to justify a new submodule. However this is subject to endless debate and is more a matter of preference anyways, so I'll let you make the final decision as to whether we keep the module or not!
There was a problem hiding this comment.
That's fine 😄
587fa8b
| /// `{revision_number: 0, revision_height: 0}` to be consistent with ibc-go, | ||
| /// where this value is used to mean "no timeout height": | ||
| /// <https://github.com/cosmos/ibc-go/blob/04791984b3d6c83f704c4f058e6ca0038d155d91/modules/core/04-channel/keeper/packet.go#L206> | ||
| pub fn compute_packet_commitment( |
There was a problem hiding this comment.
I think we can make this pub(crate) since it is only meant to be used internally by ibc-core.
| } | ||
|
|
||
| /// Compute the commitment for an acknowledgement. | ||
| pub fn compute_ack_commitment(ack: &Acknowledgement) -> AcknowledgementCommitment { |
There was a problem hiding this comment.
Same, this can be pub(crate)
| @@ -0,0 +1,3 @@ | |||
| - Export standalone commitment computations for `PacketCommitment` and | |||
There was a problem hiding this comment.
We would change this to something more along the lines of "made packet/ack commitment methods private"
…wledgementCommitment` (#492) * Export standalone commitment creators for Ack & Packet * Remove compute mod + make compute functions private * Fix clippy catch
Closes: #470, #375
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.