IBC Rate limiting contract #2408

nicolaslara · 2022-08-15T07:30:37Z

Partial work for: #2305

What is the purpose of the change

Add the contract to be used for IBC rate limiting

Brief Changelog

(for example:)

The metadata is stored in the blob store on job creation time as a persistent artifact
Deployments RPC transmits only the blob storage reference
Daemons retrieve the RPC data from the blob cache

Testing and Verifying

There are unit tests and integration tests in the contract

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (no)
How is the feature or change documented? (not applicable)

x/ibc-rate-limit/.beaker/state.json

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

p0mvn

This is really interesting, great work @nicolaslara . I'm still ramping up on the core concepts but had some initial set of questions

.github/workflows/contracts.yml

x/ibc-rate-limit/.beaker/state.json

x/ibc-rate-limit/.gitignore

p0mvn · 2022-08-16T03:29:51Z

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

+        return Err(ContractError::Unauthorized {});
+    }
+
+    let channels = CHANNEL_FLOWS.may_load(deps.storage, &channel_id)?;


asking to learn - is CHANNEL_FLOWS a whitelist of some kind?

CHANNEL_FLOWS is the main state of this contract (

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

Line 114 in a6cf294

pub const CHANNEL_FLOWS: Map<&str, Vec<ChannelFlow>> = Map::new("flow");

).

It maps the channel id to its "flows" which are: how much value has moved through the channel during the currently active time period (this week, for example), and a "quota" which represents what percentage of the channel's value (balance+locked value on osmosis) we are allowing to flow through that channel in a specific duration (1w, for example)

p0mvn · 2022-08-16T03:38:01Z

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

+    pub period_end: Timestamp,
+}
+
+impl Flow {


Can we document what this is exactly please? I'm not very familiar with IBC concepts so it would be amazing having docs for some of these abstractions

yes! a flow isn't really an IBC concept but something specific to rate limiting, but I'll add comments (and maybe a readme?) to document this.

Added some docs to state.rs

a few of the names have changed, but aded doc comments everywhere.

ValarDragon · 2022-08-16T04:28:43Z

Should we have the IBC interface functions be under Cosmwasm Sudo message? https://docs.cosmwasm.com/docs/1.0/smart-contracts/sudo/

(SendPacket, RecvPacket, AddChannel, CloseChannel)

nicolaslara · 2022-08-16T08:17:32Z

Should we have the IBC interface functions be under Cosmwasm Sudo message? https://docs.cosmwasm.com/docs/1.0/smart-contracts/sudo/

(SendPacket, RecvPacket, AddChannel, CloseChannel)

The reason to add it as regular execute messages instead of sudo is that they can be executed by someone other than the chain. Right now we define who can execute them when we instantiate the contract (

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

Lines 104 to 107 in a6cf294

    
           /// Only this module can manage the contract 
        
           pub const GOVMODULE: Item<Addr> = Item::new("gov_module"); 
        
           /// Only this module can execute transfers 
        
           pub const IBCMODULE: Item<Addr> = Item::new("ibc_module");

).

Permissions are then checked so that only the gov module is allowed to make changes and only the transfer module is allowed to send/recv packets.

My thought process is that even if these values will most likely be set to the module addresses, we could set them to something else (say, a multisig) if necessary.

ValarDragon · 2022-08-16T14:40:47Z

That makes sense. Wdyt about send packet and recv packet being sudo though? I feel like we have an api leak if its called by something not in the state machine, right?

nicolaslara · 2022-08-16T17:09:04Z

That makes sense. Wdyt about send packet and recv packet being sudo though? I feel like we have an api leak if its called by something not in the state machine, right?

yeah, that makes sense. I'll make the change.

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

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

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

…labs/osmosis into nicolas/ibc-rate-limit-contract

ValarDragon · 2022-08-17T08:04:49Z

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

 #[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.


Highlighting question

That's a really good question. Internally, cosmwasm uses Uint128 (bank queries, for example): https://github.com/CosmWasm/cosmwasm/blob/main/packages/std/src/coins.rs#L10. So that's why I went with 128.

Their implementation is basically u128 with some helpers attached.

It should be easy to change to larger representations though. Cosmwasm has Uint256 and Uint512.

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

ValarDragon · 2022-08-17T08:05:35Z

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

@@ -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 {


What do you think about moving Channel_flows then channelFlow definition to the top of this file?

Usually cosmwasm contracts have the state definition at the bottom of the state file, but we could move this around or maybe split the data structures into a separate file

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

ValarDragon · 2022-08-17T08:15:34Z

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

+        // Receiving 1% should fail. 3% already executed through the channel
+        let recv_msg = ExecuteMsg::RecvPacket {
+            channel_id: "channel".to_string(),
+            channel_value: 3_000,
+            funds: 30,
+        };


This feels incorrect right? (Relative to what we want)

E.g. the 1% recv here should be going to -1% of what we had the prior week, rather than total net change

The way it's currently implemented it's tracking only one balance. In that case, 3% has moved out of the chain, so if 1% were to move in, we're be at a 2% balance, so that's not allowed. The opposite would work: with quotas={send: 3%, recv: 2%), even if you've already sent 3%, you could receive 1% cause that would put the balance at 2%.

We could change the implementation to what you're describing though. This probably makes more sense if the quotas aren't going to be symetric. I think the "balance" implementation is a leftover from when I had the same quota for send and receive.

going to -1% of what we had the prior week

Right now we're not tracking "what we had the previous week". Instead, the percentages are calculated based on the value of the channel at the time of the request. The current implementation actually allows more than the specified percentage (as the value grows/shrinks with every send) but the actual max converges pretty quickly for small percentages. The advantage is that we avoid tracking the the network value for each channel.

Happy to change the implementation here. I think both make sense

I've just pushed a change so that this behaves as you expected. It's a bit less clean, as we need to consider both in and out flows, but closer to what a user would expect.

Added the change for "what we had the previous week" (caching the value in the quota on the first call or when it expires) in c2a72ad.

I find it a bit confusing that the differences in the quotas could be quite large when you have multiple quotas for a path (denom+channel). Consider, for example, if there is the supply was 100 last week, but someone trades 1M and wants to ibc 1%. Even if the daily 1% limit would allow you, the weekly still would prevent you from sending this through IBC.

Maybe this is what we want. We can have a quick discussion about the tradeoffs and pick one

oh thats a good point, that its super weird. Lets def discuss what makes most sense

ValarDragon · 2022-08-17T08:16:03Z

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

+    #[test]
+    fn consume_allowance() {


I think we should add a test ensuring that quotas roll over as expected

This is done implicitly here:

osmosis/x/ibc-rate-limit/contracts/rate-limiter/src/integration_tests.rs

Line 64 in 1f237e8

mod expiration {

, but I could check the internal state to make sure the reset actually sets the values to zero.

The reason to have that in integration test is because we need to modify the time, so we need a mock chain

ValarDragon · 2022-08-17T08:16:55Z

Left some comments with questions (some design questions, some minor code ones)

Awesome work on it!

I'm happy to merge this as long as we track these and write a spec, to unblock the other work going on for getting rate limits to happen!

@ValarDragon

… on the direction (as suggested by @ValarDragon)

…ed in CI

nicolaslara · 2022-08-22T10:55:49Z

Moved SendPacket and RecvPacket to sudo. There's a caveat worth noticing here though: when calling the contract via sudo, we don't specify a sender, which means that any module in the chain can call the contract. It's the responsibility of the go side to make sure this is not called in the wrong place.

ValarDragon

Awesome job!

ValarDragon · 2022-08-22T15:53:33Z

Lets merge once tests pass!

asalzmann · 2022-08-23T14:05:12Z

@nicolaslara we've been thinking about this problem as well, something I'm curious about - why key flows on (channel, denom)? Could a malicious actor create a new client-connection-channel and bridge assets off of the chain on a channel that's not rate-limited?

This is probably less relevant for assets that have been bridged onto the chain, but might be relevant for native assets that are minted and bridged off of the source chain.

nicolaslara · 2022-08-23T14:40:36Z

That's an interesting question @asalzmann, and I think it's the same for native and bridged assets. If the assets get sent through a suspicious channel (and thus wrapped accordingly) they just wouldn't be recognized by the other chains.

Don't get me wrong, that's still an issue, as the assets would be have been burned on Osmosis, but in such a scenario we're probably dealing with a hack that has to be remedied within the chain anyway. The bigger danger (and what rate limiting is trying to prevent) is those assets flowing through "proper" channels, where they would be indistinguishable* from others of the same denom.

If we wanted to rate-limit every channel+denom, it could be done by adding them to the list when a channel or asset is created), but that's a bit cumbersome. An alternative would be to have a global rate limit: right now, if nothing is configured, the contract allows all transfers; that could be changed so that, if nothing is configured, the contract assumes a default rate limit (say, 5% daily).

*indistinguishable is probably too strong here, as the transaction history is still available, but once they start flowing through other chains the chance of a coordinated recovery strategy is low

asalzmann · 2022-08-23T20:40:17Z

Thanks for talking through your reasoning here, it's very helpful!

If the assets get sent through a suspicious channel (and thus wrapped accordingly) they just wouldn't be recognized by the other chains.

The main question I have is how users of other chains perceive the trust properties of vouchers originating from different channels. My mental model is that the trust properties between channels are equivalent and that the trust properties of vouchers are derived from clients, although it's very possible that mental model is wrong.

A practical case where this could matter would be a DEX that allows deposits of any IBC voucher from another chain that's one hop away, and routes the asset to the associated pool by querying the denomTrace (this might be a bad design for a DEX!) Global rate limiting could definitely work!

The bigger danger (and what rate limiting is trying to prevent) is those assets flowing through "proper" channels, where they would be indistinguishable* from others of the same denom.

Agreed! I like the approach.

asalzmann · 2022-08-23T21:25:40Z

One other question @nicolaslara, what's the rationale for rate-limiting inflows?

nicolaslara · 2022-08-24T06:53:12Z

The main question I have is how users of other chains perceive the trust properties of vouchers originating from different channels. My mental model is that the trust properties between channels are equivalent and that the trust properties of vouchers are derived from clients, although it's very possible that mental model is wrong.

A practical case where this could matter would be a DEX that allows deposits of any IBC voucher from another chain that's one hop away, and routes the asset to the associated pool by querying the denomTrace (this might be a bad design for a DEX!) Global rate limiting could definitely work!

I'd be interested to know if there are chains out there that do this. By default, IBC-go sets the denom to the trace hash, which effectively makes them different coins. A chain with this design would need to either override this, or have some mechanism for mixing them (a stableswap, for example).

I think this is a larger question of IBC routing/wrapping that is still unresolved. Not sure if there's an accepted design for this, but I'd expect unwrapping IBC denoms would require asking the issuing chains to validate them (haven't thought about this enough, though)

One other question @nicolaslara, what's the rationale for rate-limiting inflows?

In the case of an infinite-mint on another chain (think terra collapse) someone could bring unlimited amounts of a token and tank the price of assets pooled against it; effectively draining the pools.

added ibc-rate-limiting contract

0a2656c

nicolaslara requested a review from a team August 15, 2022 07:30

This was referenced Aug 15, 2022

IBC Rate Limit - integration and tests #2410

Closed

IBC Rate Limiting #2339

Closed

nicolaslara self-assigned this Aug 15, 2022

added cosmwasm workflow

a6cf294

github-actions bot added the T:CI label Aug 15, 2022

ValarDragon reviewed Aug 16, 2022

View reviewed changes

x/ibc-rate-limit/.beaker/state.json Outdated Show resolved Hide resolved

ValarDragon reviewed Aug 16, 2022

View reviewed changes

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

p0mvn reviewed Aug 16, 2022

View reviewed changes

added doc comments

15cbcec

added migration msg

9f605fc

ValarDragon reviewed Aug 16, 2022

View reviewed changes

x/ibc-rate-limit/contracts/rate-limiter/src/msg.rs Outdated Show resolved Hide resolved

ValarDragon reviewed Aug 16, 2022

View reviewed changes

x/ibc-rate-limit/contracts/rate-limiter/src/contract.rs Outdated Show resolved Hide resolved

nicolaslara and others added 4 commits August 17, 2022 09:01

Update x/ibc-rate-limit/contracts/rate-limiter/src/msg.rs

26fc15b

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

removed gitkeep

a9dd5bf

COmments & questions

11c5765

Merge branch 'nicolas/ibc-rate-limit-contract' of github.com:osmosis-…

1f237e8

…labs/osmosis into nicolas/ibc-rate-limit-contract

ValarDragon reviewed Aug 17, 2022

View reviewed changes

x/ibc-rate-limit/contracts/rate-limiter/src/state.rs Outdated Show resolved Hide resolved

ValarDragon reviewed Aug 17, 2022

View reviewed changes

x/ibc-rate-limit/contracts/rate-limiter/src/contract.rs Outdated Show resolved Hide resolved

ValarDragon reviewed Aug 17, 2022

View reviewed changes

workflow without test data checks and clippy fixes

73acb2f

nicolaslara added 10 commits August 18, 2022 10:02

refactoring

fe97ee8

changes to the expiration logic so that balances are calculated based…

c6758f1

… on the direction (as suggested by @ValarDragon)

slightly slower but considerably cleanner

e1d52f4

cleanup attributes and removed redundancy when not testing

7434a44

update to edition 2021

727fef2

added comments explaining the tests

7859a55

removed .beaker

628db47

unified gitignore

9facb27

removed second gitignore

042f3f4

better doc comments

c37a968

nicolaslara force-pushed the nicolas/ibc-rate-limit-contract branch from ce2b292 to c37a968 Compare August 18, 2022 12:16

nicolaslara added 5 commits August 18, 2022 14:17

spelling

e5d72d6

spelling

d658fdd

added channel value cache

c2a72ad

leaving the attributes hard-coded until they can be properly configur…

43d6f17

…ed in CI

cleanup and reorganization

acc413b

lint

5cc1943

ValarDragon approved these changes Aug 22, 2022

View reviewed changes

Merge branch 'main' into nicolas/ibc-rate-limit-contract

364e35b

nicolaslara mentioned this pull request Aug 22, 2022

Small changes/chores for IBC rate limiting contract #2485

Closed

5 tasks

ValarDragon merged commit 4fde976 into main Aug 23, 2022

ValarDragon deleted the nicolas/ibc-rate-limit-contract branch August 23, 2022 00:02

adizere mentioned this pull request Dec 7, 2022

EPIC: Add IBC rate limiting mechanism cosmos/gaia#1945

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBC Rate limiting contract #2408

IBC Rate limiting contract #2408

nicolaslara commented Aug 15, 2022

p0mvn left a comment

p0mvn Aug 16, 2022

nicolaslara Aug 16, 2022

p0mvn Aug 16, 2022

nicolaslara Aug 16, 2022

nicolaslara Aug 16, 2022

nicolaslara Aug 18, 2022

ValarDragon commented Aug 16, 2022 •

edited

Loading

nicolaslara commented Aug 16, 2022

ValarDragon commented Aug 16, 2022

nicolaslara commented Aug 16, 2022

ValarDragon Aug 17, 2022

nicolaslara Aug 17, 2022

ValarDragon Aug 17, 2022

nicolaslara Aug 17, 2022

ValarDragon Aug 17, 2022

nicolaslara Aug 17, 2022

nicolaslara Aug 17, 2022

nicolaslara Aug 18, 2022

nicolaslara Aug 18, 2022

ValarDragon Aug 19, 2022

ValarDragon Aug 17, 2022

nicolaslara Aug 17, 2022

ValarDragon commented Aug 17, 2022 •

edited

Loading

nicolaslara commented Aug 22, 2022

ValarDragon left a comment

ValarDragon commented Aug 22, 2022

asalzmann commented Aug 23, 2022

nicolaslara commented Aug 23, 2022

asalzmann commented Aug 23, 2022 •

edited

Loading

asalzmann commented Aug 23, 2022

nicolaslara commented Aug 24, 2022

IBC Rate limiting contract #2408

IBC Rate limiting contract #2408

Conversation

nicolaslara commented Aug 15, 2022

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Aug 16, 2022 • edited Loading

nicolaslara commented Aug 16, 2022

ValarDragon commented Aug 16, 2022

nicolaslara commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Aug 17, 2022 • edited Loading

nicolaslara commented Aug 22, 2022

ValarDragon left a comment

Choose a reason for hiding this comment

ValarDragon commented Aug 22, 2022

asalzmann commented Aug 23, 2022

nicolaslara commented Aug 23, 2022

asalzmann commented Aug 23, 2022 • edited Loading

asalzmann commented Aug 23, 2022

nicolaslara commented Aug 24, 2022

ValarDragon commented Aug 16, 2022 •

edited

Loading

ValarDragon commented Aug 17, 2022 •

edited

Loading

asalzmann commented Aug 23, 2022 •

edited

Loading