feat: bridging fee middleware #899
Conversation
| @@ -74,7 +74,7 @@ func (im IBCMiddleware) eIBCDemandOrderHandler(ctx sdk.Context, rollappPacket co | |||
| // It validates the fungible token packet data, extracts the fee from the memo, | |||
| // calculates the demand order price, and creates a new demand order. | |||
| // It returns the created demand order or an error if there is any. | |||
| func (im IBCMiddleware) createDemandOrderFromIBCPacket(fungibleTokenPacketData transfertypes.FungibleTokenPacketData, | |||
| func (im IBCMiddleware) createDemandOrderFromIBCPacket(ctx sdk.Context, fungibleTokenPacketData transfertypes.FungibleTokenPacketData, | |||
There was a problem hiding this comment.
it would be really nice to cover this file with unit tests, especially since it has some critical features, and now we are adding changes without the possibility of regression testing
There was a problem hiding this comment.
for example, scenarios such as eibc_fee < bridging_fee...
| @@ -19,3 +19,8 @@ func (k Keeper) EpochIdentifier(ctx sdk.Context) (res string) { | |||
| k.paramstore.Get(ctx, types.KeyEpochIdentifier, &res) | |||
| return | |||
| } | |||
|
|
|||
| func (k Keeper) BridgingFee(ctx sdk.Context) (res sdk.Dec) { | |||
There was a problem hiding this comment.
can we not decouple BridgingFee from delayedack.Params? Correct me if I'm wrong, but I think bridging fee params do not belong in delayedack. Injecting a BridgingFeeKeeper into delayedack would be cleaner IMO
There was a problem hiding this comment.
I didn't want to create additional keeper and paramspace just for this
IMO the delayedAck module is actually "IBC support for rollapps" module
which includes:
- keeper, params, and common code
- delayed ack middleware
- bridging fee
- etc..
There was a problem hiding this comment.
I do kinda agree we should decide on what this module does
It's quickly going to become a god module
I would lean on splitting things up more
|
It would be good to link the issue and adr in the description |
x/delayedack/eibc.go
Outdated
| case commontypes.RollappPacket_ON_RECV: | ||
| bridgingFee := im.keeper.BridgingFee(ctx).MulInt(amt).TruncateInt() | ||
| if bridgingFee.GT(fee) { |
There was a problem hiding this comment.
| case commontypes.RollappPacket_ON_RECV: | |
| bridgingFee := im.keeper.BridgingFee(ctx).MulInt(amt).TruncateInt() | |
| if bridgingFee.GT(fee) { | |
| case commontypes.RollappPacket_ON_RECV: | |
| bridgingFee := im.keeper.BridgingFee(ctx).MulInt(amt).TruncateInt() | |
| if bridgingFee.GT(fee) { | |
| // We check that the fee the fulfiller makes is at least as big as the bridging fee they will have to pay later | |
| // this is to improve UX and help fulfillers not lose money. |
There was a problem hiding this comment.
Could you add a method to delayedack keeper to calculate the fee, given an amt
and DRY out with the usage here
There was a problem hiding this comment.
TBH
If there isn't a technical reason for this I'm not sure we should include it
It doesn't seem to me for delayedack to have awareness of bridging fee as a concept
And we should rely on frontend for these kind of UX wins
There was a problem hiding this comment.
It makes no sense to create a "losing" order for the market makers
There was a problem hiding this comment.
It does make sense from the code perspective though
It's arguably more code to maintain, for a problem which can be solved on the frontend level (before it becomes a TX) more easily
There was a problem hiding this comment.
I believe the general guideline of moving user UX/responsiblity off-chain makes sense in many cases and we should probably have this in mind. however as this is already implemented I'd suggest not to change it now and continue with current implementation.
| @@ -55,6 +55,7 @@ func (k Keeper) finalizeRollappPacket( | |||
|
|
|||
| switch rollappPacket.Type { | |||
| case commontypes.RollappPacket_ON_RECV: | |||
| // TODO: makes more sense to modify the packet when calling the handler, instead storing in db "wrong" packet | |||
There was a problem hiding this comment.
@omritoptix
I think we thought of that but I can't remember exactly why we didn't do it at the time
Maybe we were just in a rush I think
Do you remember?
| func validateBridgingFee(i interface{}) error { | ||
| v, ok := i.(sdk.Dec) | ||
| if !ok { | ||
| return fmt.Errorf("invalid parameter type: %T", i) | ||
| } | ||
| if v.IsNil() { | ||
| return fmt.Errorf("invalid global pool params: %+v", i) | ||
| } | ||
| if v.IsNegative() { | ||
| return fmt.Errorf("bridging fee must be positive: %s", v) | ||
| } | ||
|
|
||
| if v.GTE(sdk.OneDec()) { | ||
| return fmt.Errorf("bridging fee too large: %s", v) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func validateEpochIdentifier(i interface{}) error { | ||
| v, ok := i.(string) | ||
| if !ok { | ||
| return fmt.Errorf("invalid parameter type: %T", i) | ||
| } | ||
| if len(v) == 0 { | ||
| return fmt.Errorf("epoch identifier cannot be empty") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
would be nice to use sdkerrors
WalkthroughThe changes introduce a bridging fee middleware for IBC transfers in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Rollapp
participant IBCMiddleware
participant Keeper
User ->> Rollapp: Initiates IBC transfer
Rollapp ->> IBCMiddleware: Sends IBC packet
IBCMiddleware ->> Keeper: Extracts rollapp ID and packet data
IBCMiddleware ->> Keeper: Checks bridging fee
Keeper -->> IBCMiddleware: Returns fee validation
IBCMiddleware ->> Keeper: Creates demand order
Keeper -->> IBCMiddleware: Demand order created
IBCMiddleware ->> Rollapp: Acknowledges receipt
Rollapp -->> User: Transfer completed
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/delayedack/types/params.pb.gois excluded by!**/*.pb.go
Files selected for processing (21)
- app/app.go (3 hunks)
- app/apptesting/events.go (1 hunks)
- ibctesting/bridging_fee_test.go (1 hunks)
- ibctesting/eibc_test.go (1 hunks)
- proto/dymension/delayedack/params.proto (1 hunks)
- x/bridging_fee/events.go (1 hunks)
- x/bridging_fee/ibc_middleware.go (1 hunks)
- x/delayedack/eibc.go (4 hunks)
- x/delayedack/genesis_test.go (3 hunks)
- x/delayedack/ibc_middleware.go (5 hunks)
- x/delayedack/keeper/finalize.go (1 hunks)
- x/delayedack/keeper/fraud_test.go (1 hunks)
- x/delayedack/keeper/hooks_test.go (1 hunks)
- x/delayedack/keeper/keeper.go (2 hunks)
- x/delayedack/keeper/params.go (2 hunks)
- x/delayedack/types/params.go (2 hunks)
- x/eibc/keeper/hooks_test.go (1 hunks)
- x/eibc/keeper/msg_server.go (1 hunks)
- x/eibc/keeper/msg_server_test.go (2 hunks)
- x/eibc/types/errors.go (1 hunks)
- x/eibc/types/expected_keepers.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/bridging_fee/events.go
Additional comments not posted (28)
proto/dymension/delayedack/params.proto (1)
14-18: The protobuf definition forbridging_feeis correctly implemented with appropriate tags and custom type.x/delayedack/keeper/params.go (1)
10-10: The modifications toGetParamsand the addition of theBridgingFeemethod are correctly implemented and align with the PR objectives.Also applies to: 23-26
x/eibc/types/expected_keepers.go (1)
23-23: The addition of theBridgingFeemethod to theDelayedAckKeeperinterface is correctly implemented and necessary for the bridging fee functionality.x/eibc/types/errors.go (1)
22-22: The addition of theErrDemandOrderNotProfitableerror is correctly implemented and aligns with the PR objectives for handling non-profitable orders.app/apptesting/events.go (1)
54-54: The change froms.Require()tos.Assert()inAssertAttributesis a good practice in testing, allowing the continuation of test execution even if the assertion fails.x/delayedack/types/params.go (1)
13-19: The additions and modifications related toBridgingFeein theParamsstruct, including new keys, default values, and validation functions, are correctly implemented and crucial for the module's functionality.Also applies to: 23-23, 32-35, 41-41, 47-49, 52-69, 71-79, 84-88
x/eibc/keeper/msg_server.go (1)
46-48: This check ensures that the demand order is profitable by comparing the fee with the bridging fee. The logic appears correct and aligns with the PR's objectives.x/delayedack/genesis_test.go (1)
27-27: The tests forBridgingFeeinInitGenesisare well-implemented. They correctly handle cases with and without theBridgingFeeparameter, ensuring robust initialization.Also applies to: 33-39
x/delayedack/keeper/hooks_test.go (1)
73-73: The test for handlingBridgingFeein theAfterEpochEndhook is correctly implemented. It ensures that the system behaves as expected when the epoch ends, with theBridgingFeeparameter set.x/eibc/keeper/hooks_test.go (1)
68-68: The test for handlingBridgingFeein theAfterEpochEndhook within the EIBC module is correctly implemented. It ensures that the system behaves as expected when the epoch ends, with theBridgingFeeparameter set.x/delayedack/keeper/fraud_test.go (1)
66-66: The test for handlingBridgingFeein theAfterEpochEndhook within the DelayedAck module is correctly implemented. It ensures that the system behaves as expected when the epoch ends, with theBridgingFeeparameter set.x/bridging_fee/ibc_middleware.go (1)
55-114: The implementation ofOnRecvPacketinBridgingFeeMiddlewarecorrectly handles the calculation and deduction of the bridging fee from the packet data. This is crucial for ensuring that the bridging fee is applied correctly before further processing.ibctesting/bridging_fee_test.go (2)
26-50: The testTestNotRollappNoBridgingFeecorrectly verifies that no bridging fee is applied when transferring from a non-rollapp chain.
52-106: The testTestBridgingFeeeffectively validates the correct deduction of bridging fees on transfers from a rollapp, ensuring the system's financial integrity.x/delayedack/eibc.go (1)
Line range hint
77-133: The enhancements increateDemandOrderFromIBCPacketto ensure the eIBC fee is not smaller than the bridging fee are crucial for maintaining financial integrity and improving user experience.x/delayedack/ibc_middleware.go (3)
54-54: Enhancements in error handling and clarity inOnRecvPacketimprove the robustness of packet processing.
136-136: The updates toOnAcknowledgementPacketenhance the system's reliability by improving error handling and validation processes.
222-222: The improvements inOnTimeoutPacketensure consistent and reliable handling of timeout scenarios, enhancing overall system stability.x/delayedack/keeper/keeper.go (1)
82-110: The updates toExtractRollappIDAndTransferPacketenhance the accuracy and reliability of packet processing by improving error handling and validation.x/eibc/keeper/msg_server_test.go (3)
139-141: The test case for already fulfilled demand orders is well-structured and covers the expected scenario appropriately.
162-164: The test case for demand orders with a status other than pending is correctly handled and tested.
186-188: The test case for non-profitable demand orders is well-implemented, ensuring that such orders are not fulfilled.ibctesting/eibc_test.go (3)
43-43: The addition ofBridgingFeeto the test setup is correctly implemented.
43-43: Comprehensive testing of demand order creation under various scenarios enhances the robustness of the feature.
43-43: The test for demand order fulfillment is well-structured and covers both successful and error scenarios effectively.app/app.go (3)
12-12: Integration ofbridging_feemodule.This import is necessary for the subsequent use of the
bridging_feemiddleware in the application's IBC setup. It aligns with the PR's objective to implement a bridging fee middleware.
746-746: Proper integration ofbridging_feemiddleware in the IBC stack.The
bridging_fee.NewIBCMiddlewareis correctly instantiated with the necessary dependencies. This setup is crucial for enforcing the bridging fee on transfers as described in the PR objectives.
1031-1031: Exclusion oftxfeestypes.ModuleNamefrom module account addresses.This change is necessary to allow the
txfeesmodule to interact with the accounts directly, which is likely related to the handling of transaction fees, including the new bridging fees.
| @@ -55,6 +55,7 @@ func (k Keeper) finalizeRollappPacket( | |||
|
|
|||
| switch rollappPacket.Type { | |||
| case commontypes.RollappPacket_ON_RECV: | |||
| // TODO: makes more sense to modify the packet when calling the handler, instead storing in db "wrong" packet | |||
There was a problem hiding this comment.
Consider addressing the TODO comment about modifying the packet when calling the handler.
This could potentially improve the clarity and correctness of the packet handling. Would you like me to open a GitHub issue to track this improvement?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- ibctesting/bridging_fee_test.go (1 hunks)
- x/bridging_fee/ibc_middleware.go (1 hunks)
- x/delayedack/eibc.go (4 hunks)
- x/delayedack/keeper/params.go (2 hunks)
- x/eibc/keeper/msg_server.go (1 hunks)
- x/eibc/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- ibctesting/bridging_fee_test.go
- x/bridging_fee/ibc_middleware.go
- x/delayedack/eibc.go
- x/delayedack/keeper/params.go
- x/eibc/keeper/msg_server.go
- x/eibc/types/expected_keepers.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- x/delayedack/eibc.go (4 hunks)
- x/delayedack/ibc_middleware.go (5 hunks)
- x/delayedack/keeper/finalize.go (1 hunks)
- x/delayedack/types/params.go (2 hunks)
- x/eibc/genesis_test.go (2 hunks)
- x/eibc/keeper/hooks_test.go (1 hunks)
- x/eibc/keeper/msg_server_test.go (3 hunks)
- x/eibc/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- x/delayedack/eibc.go
- x/delayedack/ibc_middleware.go
- x/delayedack/keeper/finalize.go
- x/delayedack/types/params.go
- x/eibc/keeper/hooks_test.go
- x/eibc/keeper/msg_server_test.go
- x/eibc/types/expected_keepers.go
Additional comments not posted (1)
x/eibc/genesis_test.go (1)
26-26: Corrected spelling ofIsFulfilledenhances code readability and consistency.Also applies to: 35-35
|
e2e tests are currently broken. we're working on a versioning scheme which would allow us to maintain previous e2e tests in case a hotfix will be required and create new e2e tests for current. |
This PR implements a
bridging fee middlewarewhich:txfeesmoduleThe middleware reads
BridgingFeeparam stored on delayedAck keeperADR: https://www.notion.so/dymension/ADR-x-Bridging-Fee-middleware-7ba8c191373f43ce81782fc759913299?pvs=4
Description
Closes #892
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdgodoccommentsSDK Checklist
maptime.Now()sendCoinand notSendCoinsFull security checklist here
----;
For Reviewer:
---;
After reviewer approval:
Summary by CodeRabbit
New Features
Bug Fixes
IsFullfilledtoIsFulfilled.Tests
Chores