COmments & questions

x/ibc-rate-limit/contracts/rate-limiter/src/contract.rs

@@ -88,6 +88,9 @@ pub struct ChannelFlowResponse {
     pub max: u128,
 }
 
+// TODO: Lets add some more docs for this, namely that channel_value is the total supply of the denom
+// Q: Is an ICS 20 transfer only 1 denom at a time, or does the caller have to split into several
+// calls if its a multi-denom ICS-20 transfer
 pub fn try_transfer(
     deps: DepsMut,
     sender: Addr,
@@ -98,6 +101,7 @@ pub fn try_transfer(
     now: Timestamp,
 ) -> Result<Response, ContractError> {
     // Only the IBCMODULE can execute transfers
+    // TODO: Should we move this to a helper method?
     let ibc_module = IBCMODULE.load(deps.storage)?;
     if sender != ibc_module {
         return Err(ContractError::Unauthorized {});
@@ -113,6 +117,7 @@ pub fn try_transfer(
 
     if !configured {
         // No Quota configured for the current channel. Allowing all messages.
+        // TODO: Should there be an attribute for it being allowed vs denied?
         return Ok(Response::new()
             .add_attribute("method", "try_transfer")
             .add_attribute("channel_id", channel_id)
@@ -124,6 +129,12 @@ pub fn try_transfer(
     let results: Result<Vec<ChannelFlowResponse>, _> = channels
         .iter_mut()
         .map(|channel| {
+            // TODO: Should we move this to more methods on ChannelFlow?
+            // e.g. new pseudocode
+            // channel.updateIfExpired(now)
+            // channel.trackSend(&direction, funds)
+            // channel.CheckRateLimit(direction.clone())?;
+            // (Or at least update for time + rename for track send. the last one is a bit of a code style nit)
             let max = channel.quota.capacity_at(&channel_value, &direction);
             if channel.flow.is_expired(now) {
                 channel.flow.expire(now, channel.quota.duration)
@@ -138,6 +149,7 @@ pub fn try_transfer(
                 });
             };
             Ok(ChannelFlowResponse {
+                // TODO: nit, can we derive channel.Clone()?
                 channel_flow: ChannelFlow {
                     quota: channel.quota.clone(),
                     flow: channel.flow.clone(),
@@ -160,6 +172,8 @@ pub fn try_transfer(
         .add_attribute("channel_id", channel_id);
 
     // Adding the attributes from each quota to the response
+    // Code style Q: Should we move attribute setting to a function on response?
+    // Rust question: How does this work with one response being an error, I'm not sure how the flow works here
     results.iter().fold(Ok(response), |acc, result| {
         Ok(acc?
             .add_attribute(

x/ibc-rate-limit/contracts/rate-limiter/src/management.rs

@@ -32,6 +32,7 @@ pub fn try_add_channel(
     quotas: Vec<QuotaMsg>,
     now: Timestamp,
 ) -> Result<Response, ContractError> {
+    // codenit: should we make a function for checking this authorization?
     let ibc_module = IBCMODULE.load(deps.storage)?;
     let gov_module = GOVMODULE.load(deps.storage)?;
     if sender != ibc_module && sender != gov_module {
@@ -67,6 +68,7 @@ pub fn try_remove_channel(
         .add_attribute("channel_id", channel_id))
 }
 
+// Reset specified quote_id for the given channel_id
 pub fn try_reset_channel_quota(
     deps: DepsMut,
     sender: Addr,
@@ -88,6 +90,7 @@ pub fn try_reset_channel_quota(
                 channel_id: channel_id.clone(),
             }),
             Some(mut flows) => {
+                // Q: What happens here if quote_id not found? seems like we return ok?
                 flows.iter_mut().for_each(|channel| {
                     if channel.quota.name == channel_id.as_ref() {
                         channel.flow.expire(now, channel.quota.duration)

x/ibc-rate-limit/contracts/rate-limiter/src/state.rs

@@ -17,16 +17,19 @@ pub enum FlowType {
     Out,
 }
 
-/// A Flow represents the transfer of value through an IBC channel duriong a
+/// A Flow represents the transfer of value through an IBC channel during a
 /// time window.
 ///
 /// It tracks inflows (transfers into osmosis) and outflows (transfers out of
 /// osmosis).
 ///
 /// The period_end represents the last point in time that for which this Flow is
 /// tracking the value transfer.
+/// TODO: Document that time windows are not rolling windows, but instead discrete repeating windows.
+/// This is a design decision chosen for gas reasons.
 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema, Copy)]
 pub struct Flow {
+    // Q: Do we have edge case issues with inflow/outflow being u128, e.g. what if a token has super high precision.
     pub inflow: u128,
     pub outflow: u128,
     pub period_end: Timestamp,
@@ -60,7 +63,7 @@ impl Flow {
     // Mutating methods
 
     /// Expire resets the Flow to start tracking the value transfer from the
-    /// moment this methos is called.
+    /// moment this method is called.
     pub fn expire(&mut self, now: Timestamp, duration: u64) {
         self.inflow = 0;
         self.outflow = 0;
@@ -119,6 +122,8 @@ impl From<&QuotaMsg> for Quota {
     }
 }
 
+// TODO: Add docs that this is the main tracker, we should be setting multiple of these per contract
+// Q: Should we rename to "ChannelRateLimiter", and rename flow to "FlowTracker"?
 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
 pub struct ChannelFlow {
     pub quota: Quota,