IBC Rate Limiting #2339
IBC Rate Limiting #2339
Conversation
…ithout GenesisSupplyOffsets
| @@ -440,6 +464,7 @@ func (appKeepers *AppKeepers) initParamsKeeper(appCodec codec.BinaryCodec, legac | |||
| paramsKeeper.Subspace(wasm.ModuleName) | |||
| paramsKeeper.Subspace(tokenfactorytypes.ModuleName) | |||
| paramsKeeper.Subspace(twaptypes.ModuleName) | |||
| paramsKeeper.Subspace(ibcratelimittypes.ModuleName) | |||
There was a problem hiding this comment.
We should also add initializing params to the upgrade handler(creating an issue for this also works for me so that we make sure we come back to this)
There was a problem hiding this comment.
Added an issue for this: #2869
| @@ -46,8 +46,8 @@ pub mod tests { | |||
| quota_name: &str, | |||
| send_recv: (u32, u32), | |||
| duration: u64, | |||
| inflow: u128, | |||
| outflow: u128, | |||
| inflow: Uint256, | |||
There was a problem hiding this comment.
just curious what made us change from u128 -> Uint256? Did we simply want to handle larger inflows and outflows?
There was a problem hiding this comment.
This is to deal with the case of a token having a very high precision: see this comment
| @@ -103,12 +103,27 @@ func (im *IBCModule) OnChanCloseConfirm( | |||
| return im.app.OnChanCloseConfirm(ctx, portID, channelID) | |||
| } | |||
|
|
|||
| func ValidateReceiverAddress(packet channeltypes.Packet) error { | |||
| var packetData transfertypes.FungibleTokenPacketData | |||
| if err := json.Unmarshal(packet.GetData(), &packetData); err != nil { | |||
There was a problem hiding this comment.
Is this simply checking if packetData can be succesfully unmarshalled without erroring?
| return gInfo, res, err | ||
| } | ||
|
|
||
| // Move epochs to the future to avoid issues with minting |
There was a problem hiding this comment.
what's the issue when tokens get minted? Also curious if we don't need epoch within these testings at all
There was a problem hiding this comment.
See this: #2409 (comment).
I considered removing the epochs, but I think it was more work that this workaround. Since it's only for this test, I figured this was enough.
| accountFrom = suite.chainA.SenderAccount.GetAddress().String() | ||
| accountTo = suite.chainB.SenderAccount.GetAddress().String() | ||
| } else { | ||
| coins = transfertypes.GetTransferCoin( |
There was a problem hiding this comment.
I think we need this to test the receive case, but I may be wrong. I'll double check
| ) | ||
| } | ||
|
|
||
| // Tests that a receiver address longer than 4096 is not accepted |
There was a problem hiding this comment.
nit: consider grouping helper methods together and the actual tests together
There was a problem hiding this comment.
I wrote it this way cause I like the helpers to be close to where they're being used, but not a strong opinion, so I can move them
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
…-integration-and-tests
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
Closes: ##2305
What is the purpose of the change
We want to add the feature of rate limiting sends on IBC channels to Osmosis. The intended goal, is to be able to cap the amount of certain assets that can flow into and out of Osmosis per unit time, to mitigate bridge risk. It should be the case that if there is some unforeseen bridge bug or infinite token mint, the max amount extractable out of Osmosis is bounded.
See issue #2305. This PR tracks the development of the IBC Rate Limiting functionality.
Brief Changelog
Testing and Verifying
This change added tests and can be verified as follows:
Documentation and Release Note
TBD
Unreleasedsection inCHANGELOG.md? (yes / no)x/<module>/spec/) / Osmosis docs repo / not documented)