fix(eIBC): bridging_fee taken from original recipient and not from fufliller #918
Conversation
WalkthroughThe changes focus on restructuring the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Keeper
participant RollappPacket
participant AccountKeeper
participant BankKeeper
User->>Keeper: Initiate EIBCDemandOrderHandler
Keeper->>RollappPacket: Validate Packet
Note over Keeper: Check if Address is Blocked
Keeper->>AccountKeeper: Create Demand Order
AccountKeeper-->>Keeper: Returns Demand Order
Keeper->>BankKeeper: Process Payment
BankKeeper-->>Keeper: Payment Success
Keeper-->>User: Demand Order Processed Successfully
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
Outside diff range and nitpick comments (1)
app/app.go (1)
655-655: Ensure that theEIBCKeeperreference is correctly managed throughout the application lifecycle.Passing
EIBCKeeperby reference allows for more flexible and efficient management of its state across different parts of the application. However, it's crucial to ensure that all parts of the application that interact withEIBCKeeperare updated to handle it as a reference to avoid runtime errors. Consider adding unit tests to cover these interactions.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- app/app.go (1 hunks)
- ibctesting/eibc_test.go (8 hunks)
- x/bridging_fee/ibc_middleware.go (2 hunks)
- x/delayedack/ibc_middleware.go (3 hunks)
- x/delayedack/keeper/keeper.go (2 hunks)
- x/delayedack/types/expected_keepers.go (2 hunks)
- x/eibc/keeper/demand_order.go (9 hunks)
- x/eibc/keeper/invariants.go (1 hunks)
- x/eibc/keeper/keeper.go (4 hunks)
- x/eibc/keeper/msg_server.go (1 hunks)
- x/eibc/keeper/msg_server_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/eibc/keeper/msg_server_test.go
Additional comments not posted (27)
x/eibc/keeper/msg_server.go (4)
46-46: Ensure proper handling of blocked addresses.This check prevents transactions involving blocked addresses, aligning with security best practices.
50-50: Validate the existence of the fulfiller's account.This validation ensures that the fulfiller's account exists before proceeding, which is crucial for the integrity of the transaction.
56-56: Ensure funds transfer logic is correct.The logic to transfer funds from the fulfiller to the recipient is crucial for the correct execution of the order.
62-65: Confirm order fulfillment updates.This segment correctly updates the order status and handles potential errors, ensuring robust error handling and state management.
x/delayedack/types/expected_keepers.go (1)
50-50: Addition ofEIBCDemandOrderHandlermethod toEIBCKeeper.This new method enhances the interface by providing a clear contract for handling EIBC demand orders, which is essential for the new functionality introduced in this PR.
x/eibc/keeper/invariants.go (1)
70-70: Ensure underlying packets for demand orders are correctly retrieved.This invariant check is crucial for maintaining the integrity of the system by ensuring that all demand orders have corresponding underlying packets.
x/bridging_fee/ibc_middleware.go (1)
102-104: Enhance event attributes for bridging fee transactions.The addition of these attributes improves the traceability and clarity of bridging fee transactions by providing detailed information about the receiver, denomination, and amount.
x/eibc/keeper/keeper.go (4)
23-25: The restructuring of theKeeperstruct to use individual fields forak,bk, anddackinstead of embedding types directly is a good practice. This change enhances clarity and control over the dependencies.
44-50: The constructor functionNewKeepercorrectly initializes theKeeperstruct with the new fields. This ensures that all dependencies are properly set up when a new instance ofKeeperis created.
109-116: The methodSetOrderFulfilledis well-implemented with clear documentation and proper error handling. The addition of a hook call after the demand order is fulfilled is a good practice, as it allows for extensible behavior.
210-210: The addition of theSetDelayedAckKeepermethod in theKeeperstruct allows for dynamic setting of theDelayedAckKeeper. This is useful for cases where the keeper needs to be swapped or reinitialized during runtime.x/eibc/keeper/demand_order.go (5)
17-23: TheBlockedAddrmethod is correctly implemented to check if an address is blocked. It uses theBankKeeper'sBlockedAddrmethod, which abstracts away the specifics of how addresses are determined to be blocked.
Line range hint
29-75: TheEIBCDemandOrderHandlermethod is well-structured and handles different packet types effectively. The use of a switch statement to handle different types of rollapp packets (ON_RECV, ON_TIMEOUT, ON_ACK) is clear and maintainable.
Line range hint
86-112: ThecreateDemandOrderFromIBCPacketmethod is comprehensive and handles the creation of demand orders robustly. It includes validation of packet data and calculation of fees, which are crucial for the integrity of demand orders.
139-145: The logic to ensure that the eIBC fee is not smaller than the bridging fee is crucial for preventing financial losses for fulfillers. This check improves the robustness of the fee handling mechanism.
159-159: ThegetEIBCTransferDenommethod correctly handles the determination of the denomination for eIBC transfers. It considers whether the receiver chain is the source and adjusts the denomination accordingly, which is important for correct value transfers across chains.x/delayedack/ibc_middleware.go (3)
105-105: The call toEIBCDemandOrderHandlerin theOnRecvPacketmethod is correctly placed to handle the creation of demand orders upon the receipt of packets. This ensures that the eIBC logic is integrated into the packet reception process.
197-197: The integration ofEIBCDemandOrderHandlerin theOnAcknowledgementPacketmethod is appropriate for handling cases where the acknowledgement is an error. This ensures that demand orders are created even in error scenarios, which is important for robust error handling.
280-280: The addition ofEIBCDemandOrderHandlerin theOnTimeoutPacketmethod is crucial for handling timeout scenarios. This ensures that demand orders are processed even when packets timeout, which is important for maintaining consistency and reliability in packet handling.x/delayedack/keeper/keeper.go (1)
160-161: The delegation of theEIBCDemandOrderHandlermethod to theEIBCKeeperis a good example of using composition over inheritance, which simplifies the architecture and makes the code easier to manage.ibctesting/eibc_test.go (7)
183-183: IntroducebankKeepervariable to improve test isolation.This change allows the test to manipulate and verify bank operations directly, enhancing test reliability and clarity.
216-216: Ensure correct handling of initial balances.Retrieving the initial balance of the IBC original recipient is crucial for validating the state changes accurately in the test.
245-245: Check the fulfiller's balance after finalization.This step is essential for ensuring that the fulfiller's balance is updated correctly after the transaction, which is critical for the integrity of the test.
288-288: Validate the demand order fulfillment logic.This line initiates the process of fulfilling a demand order, which is central to the test's objective of verifying the eIBC functionality.
310-311: Verify the balances of fulfiller and recipient after transactions.These lines are crucial for asserting that the balances are updated correctly post-transaction, which is a key aspect of the test's integrity.
326-326: Check the fulfiller's balance after the rollapp state finalization.This ensures that the test verifies the final state of the fulfiller's balance, confirming the correct application of transaction effects.
372-372: IntroducebankKeepervariable in the timeout test scenario.Similar to the earlier introduction of
bankKeeper, this change allows for direct manipulation and verification of bank operations in the context of timeout handling, improving test accuracy.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/eibc/keeper/keeper.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/eibc/keeper/keeper.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 (1)
- x/eibc/keeper/demand_order.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/eibc/keeper/demand_order.go
…en-from-original-recipient-and-not-from-fufliller-when-using-eibc
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)
- app/app.go (1 hunks)
- ibctesting/eibc_test.go (8 hunks)
- x/bridging_fee/ibc_middleware.go (2 hunks)
- x/delayedack/ibc_middleware.go (3 hunks)
- x/delayedack/keeper/keeper.go (2 hunks)
- x/delayedack/types/expected_keepers.go (2 hunks)
- x/eibc/keeper/invariants.go (1 hunks)
- x/eibc/keeper/keeper.go (4 hunks)
Files skipped from review as they are similar to previous changes (8)
- app/app.go
- ibctesting/eibc_test.go
- x/bridging_fee/ibc_middleware.go
- x/delayedack/ibc_middleware.go
- x/delayedack/keeper/keeper.go
- x/delayedack/types/expected_keepers.go
- x/eibc/keeper/invariants.go
- x/eibc/keeper/keeper.go
| if amt.LT(fee) { | ||
| return nil, fmt.Errorf("fee cannot be larger than amount: fee: %s: amt :%s", fee, fungibleTokenPacketData.Amount) | ||
| } | ||
|
|
||
| // Get the bridging fee from the amount | ||
| bridgingFee := k.dack.BridgingFeeFromAmt(ctx, amt) | ||
| demandOrderPrice := amt.Sub(fee).Sub(bridgingFee) |
There was a problem hiding this comment.
isn't it possible that the demandOrderPrice can be zero or less? I think the check on line 105: if amt.LT(fee) should include the bridgingFee as well
There was a problem hiding this comment.
also, it would be nice to cover this with tests
There was a problem hiding this comment.
added check.
UT will be handled in #916
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/eibc/keeper/demand_order.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/eibc/keeper/demand_order.go
| if amt.LT(fee) { | ||
| return nil, fmt.Errorf("fee cannot be larger than amount: fee: %s: amt :%s", fee, fungibleTokenPacketData.Amount) | ||
| } | ||
|
|
||
| // Get the bridging fee from the amount | ||
| bridgingFee := k.dack.BridgingFeeFromAmt(ctx, amt) |
There was a problem hiding this comment.
onTimeout and AckErr shouldn't charge bridging fee, only OnRecv
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- x/common/types/rollapp_packet.go (1 hunks)
- x/eibc/keeper/demand_order.go (1 hunks)
Additional comments not posted (5)
x/common/types/rollapp_packet.go (1)
11-14: The newLogString()method provides a clear, formatted string ofRollappPacket's important fields, which enhances logging and debugging capabilities.x/eibc/keeper/demand_order.go (4)
1-1: Changing the package name fromdelayedacktokeeperaligns with the restructuring of the module, promoting clarity and consistency.
25-58: TheEIBCDemandOrderHandlermethod has been updated to handle different types ofrollappPacket. The logic is well-structured, separating concerns based on packet type, which enhances readability and maintainability.
60-107: ThecreateDemandOrderFromIBCPacketfunction is robust, with comprehensive error handling and validations. It correctly handles various scenarios, including blocked addresses and validation of the metadata and amount.
109-190: ThePrepareProtocolDemandOrderfunction has been updated to handle timeout and ack error scenarios. The method is well-documented and follows good coding practices by validating inputs and handling errors appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/eibc/keeper/demand_order.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/eibc/keeper/demand_order.go
x/common/types/rollapp_packet.go
Outdated
| func (r RollappPacket) LogString() string { | ||
| return fmt.Sprintf("RollappPacket{%s, %s, %s, %s, %s, %d}", | ||
| r.RollappId, r.Type, r.Status, r.Packet.SourcePort, r.Packet.SourceChannel, r.Packet.Sequence) | ||
| } |
There was a problem hiding this comment.
please add error and proof height
x/delayedack/keeper/keeper.go
Outdated
| func (k Keeper) EIBCDemandOrderHandler(ctx sdk.Context, rollappPacket commontypes.RollappPacket, transferPacketData transfertypes.FungibleTokenPacketData) error { | ||
| return k.EIBCKeeper.EIBCDemandOrderHandler(ctx, rollappPacket, transferPacketData) |
x/eibc/keeper/demand_order.go
Outdated
| /* | ||
| In case of timeout/errack: | ||
|
|
||
| fee = fee_multiplier*transfer_amount | ||
| price = transfer_amount-fee | ||
| order is created with (price,fee) | ||
| the order creator immediately receives price on fulfillment from fulfiller | ||
| when the ack/timeout finalizes, the fulfiller receives the transfer_amount | ||
|
|
||
| therefore: | ||
| fulfiller balance += (transfer_amount - (transfer_amount-fee)) | ||
| equivalent to += fee_multiplier*transfer_amount | ||
| demander balance += (transfer_amount - fee) | ||
| equivalent to += (1-fee_multiplier)*transfer_amount | ||
| */ |
x/eibc/keeper/demand_order.go
Outdated
| // Validate the fungible token packet data as we're going to use it to create the demand order | ||
| if err := fungibleTokenPacketData.ValidateBasic(); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
its already validated right?
x/eibc/keeper/demand_order.go
Outdated
| order := eibctypes.NewDemandOrder(*rollappPacket, demandOrderPrice, fee, demandOrderDenom, demandOrderRecipient) | ||
| if err := order.Validate(); err != nil { | ||
| return nil, fmt.Errorf("validate eibc data: %w", err) | ||
| } | ||
| return order, nil |
There was a problem hiding this comment.
duplicated, could be dried out
There was a problem hiding this comment.
dried out some
but the creation I prefer to do in the inner func, otherwise I need to return all args (demandOrderPrice, fee, demandOrderDenom, demandOrderRecipient)
|
I would say though it's getting very hard and confusing to follow for me We should refactor at some point to just have 1 very clear calculation in the code somehwere |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- x/common/types/rollapp_packet.go (1 hunks)
- x/delayedack/keeper/keeper.go (2 hunks)
- x/eibc/keeper/demand_order.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/common/types/rollapp_packet.go
- x/delayedack/keeper/keeper.go
- x/eibc/keeper/demand_order.go
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
x/eibc/keeper/demand_order.go (1)
17-21: This function's documentation effectively outlines its responsibilities and the conditions under which it operates. However, it could be enhanced by specifying what types of errors might be returned and under what conditions.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/eibc/keeper/demand_order.go (1 hunks)
Additional comments not posted (1)
x/eibc/keeper/demand_order.go (1)
27-33: Excellent error handling for basic validation and blocked address checks. This ensures that only valid and permitted operations proceed, enhancing the security and integrity of the transactions.
mtsitrin
deleted the
mtsitrin/917-change-ibc-bridge-fee-to-be-taken-from-original-recipient-and-not-from-fufliller-when-using-eibc
branch
…not from fufliller #918 - everything broken
This PR moves the "bridging fee" tax from the fulfiller to the original destination address of the transfer.
So when a demand order created:
instead of currently:
So the fulfiller sends the
Priceon fulfillment, and getsAmt **- bridging fee**(==fee)Closes #917
Summary by CodeRabbit
New Features
EIBCDemandOrderHandlerto manage EIBC packets by creating demand orders based on packet data.LogStringmethod forRollappPacketto generate formatted strings of packet details.BlockedAddrmethod to check for blocked addresses.Refactor
delayedacktokeeper.Style