modules: Refactor reference-to-tuple method args #2603
Conversation
In core::ics04_channel::{ChannelReader, ChannelKeeper},
rework method arguments that force the caller to make unnecessary
clones or form tuples for no good reason, breaking them up into
individual arguments. Change some arguments on store_* methods
from passed by reference to moved because the implementation is expected
to take ownership of them. On delete_* methods, the key arguments
need to be passed by reference.
mzabaluev
added
O: new-feature
|
|
||
| // Used by unordered channel | ||
| pub packet_receipt: BTreeMap<(PortId, ChannelId, Sequence), Receipt>, | ||
| pub packet_receipt: PortChannelIdMap<BTreeMap<Sequence, Receipt>>, |
There was a problem hiding this comment.
Nit: I think it would be better to use BTreeMap<ibc::core::ics24_host::path::ReceiptsPath, Receipt> in such cases or provide a PortChannelIdMap::get() that hides the double/triple look up.
There was a problem hiding this comment.
So far PortChannelIdMap is just a type alias, so I was hesitant to add method API around it. Maybe standalone helper fns would be the simplest way to deal with it; I don't want to invent an extension trait or do a newtype just for a convenience tradeoff.
There was a problem hiding this comment.
I think it would be better to use
BTreeMap<ibc::core::ics24_host::path::ReceiptsPath, Receipt>
A non-allocating API for this would force use of ReceiptsPath in method parameters for get_packet_receipt/store_packet_receipt, and the call sites do not readily support that: the PortId, ChannelId, Sequence triplet comes from some larger structure like Packet. I suppose grouping of these packet fields as ReceiptsPath inside Packet does not make sense, because it might as well be an AcksPath?
There was a problem hiding this comment.
Yeah, it's tricky to design a non-allocating API, also because some paths deal with source port/channel-id (e.g. CommitmentsPath) while others need destination port/channel-id (e.g. ReceiptsPath & AcksPath) - so grouping them together in the Packet struct wouldn't work. I have been thinking if we should have references in all path structs in the future but that would be problematic for the FromStr impl ->
pub struct ReceiptsPath<'a> {
pub port_id: &'a PortId,
pub channel_id: &'a ChannelId,
pub sequence: Sequence,
}There was a problem hiding this comment.
I gave it some thought and tried to work with some reference-containing structs too, but quickly gave it up as impractical.
Oddly, it's one instance where the internal implementation, in conjunction with Rust associative container lookup APIs, comes at odds with API ergonomics. If anyone can suggest how to organize multi-value key containers better, let it be known.
| @@ -24,7 +24,7 @@ use super::timeout::TimeoutHeight; | |||
| /// A context supplying all the necessary read-only dependencies for processing any `ChannelMsg`. | |||
| pub trait ChannelReader { | |||
| /// Returns the ChannelEnd for the given `port_id` and `chan_id`. | |||
| fn channel_end(&self, port_channel_id: &(PortId, ChannelId)) -> Result<ChannelEnd, Error>; | |||
| fn channel_end(&self, port_id: &PortId, channel_id: &ChannelId) -> Result<ChannelEnd, Error>; | |||
In core::ics04_channel::{ChannelReader, ChannelKeeper},
rework method arguments that force the caller to make unnecessary
clones or form tuples for no good reason, breaking them up into
individual arguments. Change some arguments on store_* methods
from passed by reference to moved because the implementation is expected
to take ownership of them. On delete_* methods, the key arguments
need to be passed by reference.
Co-authored-by: hu55a1n1 <[email protected]>
In
core::ics04_channel::{ChannelReader, ChannelKeeper}, rework method arguments that force the caller to make unnecessary clones or form tuples for no good reason, breaking them up into individual arguments. Change some arguments onstore_*methods from passed by reference to moved because the implementation is expected to take ownership of them. Ondelete_*methods, the key arguments need to be passed by reference.PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.