Properly parsing denoms (#3304)

* properly parsing denoms

* fixing lints

* correct bytecode

x/ibc-rate-limit/README.md

@@ -185,6 +185,72 @@ Sudo messages can only be executed by the chain.
 * RecvPacket - Increments the amount used out of the receive quota and checks that the receive is allowed. If it isn't, it will return a RateLimitExceeded error
 * UndoSend - If a send has failed, the undo message is used to remove its cost from the send quota
 
+All of these messages receive the packet from the chain and extract the necessary information to process the packet and determine if it should be the rate limited. 
+
+### Necessary information 
+
+To determine if a packet should be rate limited, we need:
+
+* Channel: The channel on the Osmosis side: `packet.SourceChannel` for sends, and `packet.DestinationChannel` for receives. 
+* Denom: The denom of the token being transferred as known on the Osmosis side (more on that bellow)
+* Channel Value: The total value of the chanel denominated in `Denom` (i.e.: channel-17 is worth 10k osmo).  
+* Funds: the amount being transferred
+
+#### Notes on Channel
+The contract also supports quotas on a custom channel called "any" that is checked on every transfer. If either the 
+transfer channel or the "any" channel have a quota that has been filled, the transaction will be rate limited.
+
+#### Notes on Denom
+We always use the the denom as represented on Osmosis. For native assets that is the local denom, and for non-native 
+assets it's the "ibc" prefix and the sha256 hash of the denom trace (`ibc/...`).
+
+##### Sends
+
+For native denoms, we can just use the denom in the packet. If the denom is invalid, it will fail somewhere else along the chain. Example result: `uosmo`
+
+For non-native denoms, the contract needs to hash the denom trace and append it to the `ibc/` prefix. The
+contract always receives the parsed denom (i.e.: `transfer/channel-32/uatom` instead of
+`ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2`). This is because of the order in which 
+the middleware is called. When sending a non-native denom, the packet contains `transfer/source-channel/denom` as it
+is built on the `relay.SendTransfer()` in the transfer module and then passed to the middleware. Example result: `ibc/<hash>`
+
+##### Receives
+
+This behaves slightly different if the asset is an osmosis asset that was sent to the counterparty and is being
+returned to the chain, or if the asset is being received by the chain and originates on the counterparty. In ibc this
+is called being a "source" or a "sink" respectively.
+
+If the chain is a sink for the denom, we build the local denom by prefixing the port and the channel 
+(`transfer/local-channel`) and hashing that denom. Example result: `ibc/<hash>`
+
+If the chain is the source for the denom, there are two possibilities:
+
+* The token is a native token, in which case we just remove the prefix added by the counterparty. Example result: `uosmo`
+* The token is a non-native token, in which case we remove the extra prefix and hash it. Example result `ibc/<hash>`
+
+#### Notes on Channel Value
+We have iterated on different strategies for calculating the channel value. Our preferred strategy is the following:
+* For non-native tokens (`ibc/...`), the channel value should be the supply of those tokens in Osmosis
+* For native tokens, the channel value should be the total amount of tokens in escrow across all ibc channels
+
+The later ensures the limits are lower and represent the amount of native tokens that exist outside Osmosis. This is 
+beneficial as we assume the majority of native tokens exist on the native chain and the amount "normal" ibc transfers is 
+proportional to the tokens that have left the chain. 
+
+This strategy cannot be implemented at the moment because IBC does not track the amount of tokens in escrow across 
+all channels ([github issue](https://github.com/cosmos/ibc-go/issues/2664)). Instead, we use the current supply on 
+Osmosis for all denoms (i.e.: treat native and non-native tokens the same way). Once that ticket is fixed, we will 
+update this strategy.
+
+##### Caching
+
+The channel value varies constantly. To have better predictability, and avoid issues of the value growing if there is 
+a potential infinite mint bug, we cache the channel value at the beginning of the period for every quota.
+
+This means that if we have a daily quota of 1% of the osmo supply, and the channel value is 1M osmo at the beginning of 
+the quota, no more than 100k osmo can transferred during that day. If 10M osmo were to be minted or IBC'd in during that
+period, the quota will not increase until the period expired. Then it will be 1% of the new channel value (~11M)
+
 ### Integration
 
 The rate limit middleware wraps the `transferIBCModule` and is added as the entry route for IBC transfers.
@@ -196,11 +262,30 @@ This integration can be seen in [osmosis/app/keepers/keepers.go](https://github.
 
 ## Testing strategy
 
-TODO: Fill this out
+
+A general testing strategy is as follows:
+
+* Setup two chains.
+* Send some tokens from A->B and some from B->A (so that there are IBC tokens to play with in both sides)
+* Add the rate limiter on A with low limits (i.e. 1% of supply)
+* Test Function for chains A' and B' and denom d
+  * Send some d tokens from A' to B' and get close to the limit. 
+  * Do the same transfer making sure the amount is above the quota and verify it fails with the rate limit error
+  * Wait until the reset time has passed, and send again. The transfer should now succeed
+* Repeat the above test for the following combination of chains and tokens: `(A,B,a)`, `(B,A,a)`, `(A,B,b)`, `(B,A,b)`, 
+  where `a` and `b` are native tokens to chains A and B respectively.
+
+For more comprehensive tests we can also:
+* Add a third chain C and make sure everything works properly for C tokens that have been transferred to A and to B
+* Test that the contracts gov address can reset rate limits if the quota has been hit
+* Test the queries for getting information about the state of the quotas 
+* Test that rate limit symmetries hold (i.e.: sending the a token through a rate-limited channel and then sending back 
+  reduces the rate limits by the same amount that it was increased during the first send)
+* Ensure that the channels between the test chains have different names (A->B="channel-0", B->A="channel-1", for example)
 
 ## Known Future work
 
-Items that've been highlighted above:
+Items that have been highlighted above:
 
 * Making automated rate limits get added for channels, instead of manual configuration only
 * Improving parameterization strategies / data analysis

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

@@ -77,6 +77,14 @@ pub struct QuerySupplyOfResponse {
 
 use std::str::FromStr; // Needed to parse the coin's String as Uint256
 
+fn hash_denom(denom: &str) -> String {
+    let mut hasher = Sha256::new();
+    hasher.update(denom.as_bytes());
+    let result = hasher.finalize();
+    let hash = hex::encode(result);
+    format!("ibc/{}", hash.to_uppercase())
+}
+
 impl Packet {
     pub fn mock(channel_id: String, denom: String, funds: Uint256) -> Packet {
         Packet {
@@ -99,9 +107,9 @@ impl Packet {
         }
     }
 
-    pub fn channel_value(&self, deps: Deps) -> Result<Uint256, StdError> {
+    pub fn channel_value(&self, deps: Deps, direction: &FlowType) -> Result<Uint256, StdError> {
         let res = QuerySupplyOfRequest {
-            denom: self.local_denom(),
+            denom: self.local_denom(direction),
         }
         .query(&deps.querier)?;
         Uint256::from_str(&res.amount.unwrap_or_default().amount)
@@ -119,27 +127,64 @@ impl Packet {
         }
     }
 
-    fn local_denom(&self) -> String {
+    fn receiver_chain_is_source(&self) -> bool {
+        self.data
+            .denom
+            .starts_with(&format!("transfer/{}", self.source_channel))
+    }
+
+    fn handle_denom_for_sends(&self) -> String {
         if !self.data.denom.starts_with("transfer/") {
             // For native tokens we just use what's on the packet
             return self.data.denom.clone();
         }
         // For non-native tokens, we need to generate the IBCDenom
-        let mut hasher = Sha256::new();
-        hasher.update(self.data.denom.as_bytes());
-        let result = hasher.finalize();
-        let hash = hex::encode(result);
-        format!("ibc/{}", hash.to_uppercase())
+        hash_denom(&self.data.denom)
+    }
+
+    fn handle_denom_for_recvs(&self) -> String {
+        if self.receiver_chain_is_source() {
+            // These are tokens that have been sent to the counterparty and are returning
+            let unprefixed = self
+                .data
+                .denom
+                .strip_prefix(&format!("transfer/{}/", self.source_channel))
+                .unwrap_or_default();
+            let split: Vec<&str> = unprefixed.split('/').collect();
+            if split[0] == unprefixed {
+                // This is a native token. Return the unprefixed token
+                unprefixed.to_string()
+            } else {
+                // This is a non-native that was sent to the counterparty.
+                // We need to hash it.
+                // The ibc-go implementation checks that the denom has been built correctly. We
+                // don't need to do that here because if it hasn't, the transfer module will catch it.
+                hash_denom(unprefixed)
+            }
+        } else {
+            // Tokens that come directly from the counterparty.
+            // Since the sender didn't prefix them, we need to do it here.
+            let prefixed = format!("transfer/{}/", self.destination_channel) + &self.data.denom;
+            hash_denom(&prefixed)
+        }
+    }
+
+    fn local_denom(&self, direction: &FlowType) -> String {
+        match direction {
+            FlowType::In => self.handle_denom_for_recvs(),
+            FlowType::Out => self.handle_denom_for_sends(),
+        }
     }
 
     pub fn path_data(&self, direction: &FlowType) -> (String, String) {
-        (self.local_channel(direction), self.local_denom())
+        (self.local_channel(direction), self.local_denom(direction))
     }
 }
 
 // Helpers
 
 // Create a new packet for testing
+#[cfg(test)]
 #[macro_export]
 macro_rules! test_msg_send {
     (channel_id: $channel_id:expr, denom: $denom:expr, channel_value: $channel_value:expr, funds: $funds:expr) => {
@@ -151,6 +196,7 @@ macro_rules! test_msg_send {
     };
 }
 
+#[cfg(test)]
 #[macro_export]
 macro_rules! test_msg_recv {
     (channel_id: $channel_id:expr, denom: $denom:expr, channel_value: $channel_value:expr, funds: $funds:expr) => {

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

@@ -31,7 +31,7 @@ pub fn process_packet(
     let funds = packet.get_funds();
     let channel_value = match channel_value_hint {
         Some(channel_value) => channel_value,
-        None => packet.channel_value(deps.as_ref())?,
+        None => packet.channel_value(deps.as_ref(), &direction)?,
     };
     try_transfer(deps, path, channel_value, funds, direction, now)
 }

x/ibc-rate-limit/ibc_middleware_test.go

@@ -308,6 +308,7 @@ func (suite *MiddlewareTestSuite) fullSendTest(native bool) map[string]string {
 	suite.chainA.RegisterRateLimitingContract(addr)
 
 	// send 2.5% (quota is 5%)
+	fmt.Printf("Sending %s from A to B. Represented in chain A as wrapped? %v\n", denom, !native)
 	suite.AssertSend(true, suite.MessageFromAToB(denom, sendAmount))
 
 	// send 2.5% (quota is 5%)
@@ -329,11 +330,15 @@ func (suite *MiddlewareTestSuite) fullSendTest(native bool) map[string]string {
 
 // Test rate limiting on sends
 func (suite *MiddlewareTestSuite) TestSendTransferWithRateLimitingNative() {
+	// Sends denom=stake from A->B. Rate limit receives "stake" in the packet. Nothing to do in the contract
 	suite.fullSendTest(true)
 }
 
 // Test rate limiting on sends
 func (suite *MiddlewareTestSuite) TestSendTransferWithRateLimitingNonNative() {
+	// Sends denom=ibc/C053D637CCA2A2BA030E2C5EE1B28A16F71CCB0E45E8BE52766DC1B241B77878 from A->B.
+	// Rate limit receives "transfer/channel-0/stake" in the packet (because transfer.relay.SendTransfer is called before the middleware)
+	// and should hash it before calculating the value
 	suite.fullSendTest(false)
 }
 
@@ -362,48 +367,58 @@ func (suite *MiddlewareTestSuite) TestSendTransferReset() {
 
 // Test rate limiting on receives
 func (suite *MiddlewareTestSuite) fullRecvTest(native bool) {
-	quotaPercentage := 5
+	quotaPercentage := 4
 	suite.initializeEscrow()
 	// Get the denom and amount to send
-	denom := sdk.DefaultBondDenom
+	sendDenom := sdk.DefaultBondDenom
+	localDenom := sdk.DefaultBondDenom
 	channel := "channel-0"
-	if !native {
-		denomTrace := transfertypes.ParseDenomTrace(transfertypes.GetPrefixedDenom("transfer", "channel-0", denom))
-		denom = denomTrace.IBCDenom()
+	if native {
+		denomTrace := transfertypes.ParseDenomTrace(transfertypes.GetPrefixedDenom("transfer", "channel-0", localDenom))
+		localDenom = denomTrace.IBCDenom()
+	} else {
+		denomTrace := transfertypes.ParseDenomTrace(transfertypes.GetPrefixedDenom("transfer", "channel-0", sendDenom))
+		sendDenom = denomTrace.IBCDenom()
 	}
 
 	osmosisApp := suite.chainA.GetOsmosisApp()
 
-	// This is the first one. Inside the tests. It works as expected.
-	channelValue := CalculateChannelValue(suite.chainA.GetContext(), denom, osmosisApp.BankKeeper)
+	channelValue := CalculateChannelValue(suite.chainA.GetContext(), localDenom, osmosisApp.BankKeeper)
 
-	// The amount to be sent is send 2.5% (quota is 5%)
+	// The amount to be sent is 2% (quota is 4%)
 	quota := channelValue.QuoRaw(int64(100 / quotaPercentage))
 	sendAmount := quota.QuoRaw(2)
 
-	fmt.Printf("Testing recv rate limiting for denom=%s, channelValue=%s, quota=%s, sendAmount=%s\n", denom, channelValue, quota, sendAmount)
+	fmt.Printf("Testing recv rate limiting for denom=%s, channelValue=%s, quota=%s, sendAmount=%s\n", localDenom, channelValue, quota, sendAmount)
 
 	// Setup contract
 	suite.chainA.StoreContractCode(&suite.Suite)
-	quotas := suite.BuildChannelQuota("weekly", channel, denom, 604800, 5, 5)
+	quotas := suite.BuildChannelQuota("weekly", channel, localDenom, 604800, 4, 4)
 	addr := suite.chainA.InstantiateContract(&suite.Suite, quotas)
 	suite.chainA.RegisterRateLimitingContract(addr)
 
 	// receive 2.5% (quota is 5%)
-	suite.AssertReceive(true, suite.MessageFromBToA(denom, sendAmount))
+	fmt.Printf("Sending %s from B to A. Represented in chain A as wrapped? %v\n", sendDenom, native)
+	suite.AssertReceive(true, suite.MessageFromBToA(sendDenom, sendAmount))
 
 	// receive 2.5% (quota is 5%)
-	suite.AssertReceive(true, suite.MessageFromBToA(denom, sendAmount))
+	suite.AssertReceive(true, suite.MessageFromBToA(sendDenom, sendAmount))
 
 	// Sending above the quota should fail. We send 2 instead of 1 to account for rounding errors
-	suite.AssertReceive(false, suite.MessageFromBToA(denom, sdk.NewInt(2)))
+	suite.AssertReceive(false, suite.MessageFromBToA(sendDenom, sdk.NewInt(2)))
 }
 
 func (suite *MiddlewareTestSuite) TestRecvTransferWithRateLimitingNative() {
+	// Sends denom=stake from B->A.
+	// Rate limit receives "stake" in the packet and should wrap it before calculating the value
+	// types.ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), data.Denom) should return false => Wrap the token
 	suite.fullRecvTest(true)
 }
 
 func (suite *MiddlewareTestSuite) TestRecvTransferWithRateLimitingNonNative() {
+	// Sends denom=ibc/C053D637CCA2A2BA030E2C5EE1B28A16F71CCB0E45E8BE52766DC1B241B77878 from B->A.
+	// Rate limit receives "transfer/channel-0/stake" in the packet and should turn it into "stake"
+	// types.ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), data.Denom) should return true => unprefix. If unprefixed is not local, hash.
 	suite.fullRecvTest(false)
 }
 

x/ibc-rate-limit/ibc_module.go

@@ -2,6 +2,7 @@ package ibc_rate_limit
 
 import (
 	"encoding/json"
+	"strings"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -132,8 +133,10 @@ func (im *IBCModule) OnRecvPacket(
 
 	err := CheckAndUpdateRateLimits(ctx, im.ics4Middleware.ContractKeeper, "recv_packet", contract, packet)
 	if err != nil {
-		// ToDo: add the full error as an event instead?
-		fullError := sdkerrors.Wrap(types.ErrRateLimitExceeded, err.Error())
+		if strings.Contains(err.Error(), "rate limit exceeded") {
+			return channeltypes.NewErrorAcknowledgement(types.ErrRateLimitExceeded.Error())
+		}
+		fullError := sdkerrors.Wrap(types.ErrContractError, err.Error())
 		return channeltypes.NewErrorAcknowledgement(fullError.Error())
 	}