Implement Taker Fee Burn Mechanism #9022
Conversation
VolodymyrBg
requested review from
nicolaslara,
p0mvn,
AlpinYukseloglu,
czarcas7ic and
mattverse
as code owners
github-actions
bot
added
C:x/txfees
C:app-wiring
WalkthroughThis pull request introduces changes to support burning of taker fees. A new module account is added in the permissions map, new fields are incorporated in protocol buffer definitions to handle burn percentages and track burned fees, and additional keeper methods facilitate fee tracking and updates. New constants and variables are introduced for tracking and telemetry, and the fee distribution logic has been extended with conditional checks, swapping, and direct burn transfers for both OSMO and non-OSMO fees. Changes
Sequence Diagram(s)sequenceDiagram
participant FC as Fee Calculation (hooks)
participant MA as Taker Fee Module Account
participant BA as Burn Address
participant SW as Swap Module (Non-OSMO)
FC->>MA: Check fee balance & Burn parameter
alt OSMO fees available
FC->>MA: Calculate burn amount for OSMO
FC->>BA: Transfer OSMO coins for burn
MA-->>FC: Update module account balance
else Non-OSMO fees
FC->>MA: Verify fee balance and whitelist status
alt Denom is whitelisted
FC->>BA: Execute direct burn transfer for non-OSMO
else Non-whitelisted denom
FC->>MA: Track fees intended for burn
FC->>SW: Initiate swap to OSMO
SW->>BA: Swap & transfer coins to burn address
end
end
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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
🧹 Nitpick comments (3)
x/txfees/types/expected_keepers.go (1)
57-57: Missing method documentation.Unlike other similar methods in this interface,
UpdateTakerFeeTrackerForBurnByDenomlacks a comment explaining its purpose and behavior. This is inconsistent with the codebase style.Add a descriptive comment following the pattern of the similar methods:
+ // UpdateTakerFeeTrackerForBurnByDenom updates the taker fee tracker for burn by increasing the amount for a specific denom UpdateTakerFeeTrackerForBurnByDenom(ctx sdk.Context, denom string, increasedAmt osmomath.Int) errorx/txfees/keeper/hooks.go (2)
167-200: Non-OSMO burn logic properly differentiates whitelisted vs. non-whitelisted assets.This approach effectively handles direct burn of whitelisted assets and defers unwhitelisted assets for a later swap. Ensure robust coverage in test cases for edge scenarios (e.g., zero or extremely small amounts, or no liquidity for swap).
262-277: Final burn after swapping non-whitelisted assets is well-structured.The logic mirrors the earlier direct-burn steps for whitelisted assets but includes the final swap step. Be sure to handle potential failures (e.g., no pool found, partial swap). Logging is already present; consider including the exact reason in logs if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/poolmanager/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (8)
app/modules.go(1 hunks)proto/osmosis/poolmanager/v1beta1/genesis.proto(2 hunks)x/poolmanager/protorev.go(1 hunks)x/poolmanager/types/keys.go(1 hunks)x/txfees/keeper/hooks.go(4 hunks)x/txfees/types/expected_keepers.go(1 hunks)x/txfees/types/keys.go(1 hunks)x/txfees/types/telemetry.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (10)
app/modules.go (1)
137-137: LGTM! Addition of TakerFeeBurnName to moduleAccountPermissions.The new module account for taker fee burning is correctly added to the permissions map with nil permissions, consistent with other fee collector module accounts.
x/txfees/types/telemetry.go (1)
38-44: LGTM! New telemetry metric for failed burn operations.The telemetry implementation correctly follows the established pattern for other taker fee metrics, providing appropriate labels for tracking failed burn operations.
x/txfees/types/keys.go (1)
28-31: New module account constant looks consistent and well-documented.The addition of
TakerFeeBurnNamealigns with the existing pattern ofTakerFeeCommunityPoolNameandTakerFeeStakersName. The docstring clearly explains the routing for scenarios where a swap fails, ensuring funds designated for burning remain isolated.proto/osmosis/poolmanager/v1beta1/genesis.proto (2)
126-131: Ensure total ratio validations for the new 'burn' field.The new
burnfield fits well with the existing distribution fields. Consider verifying in downstream logic or validations thatstaking_rewards + community_pool + burndoes not exceed 1 (100%) if there's an expectation for these distributions to be within a bounded ratio.
139-140: Consistent addition of taker_fees_burned field.This repeated coin field provides clarity in tracking burned fees. Ensure there's robust testing in place to confirm correct accumulation and usage of these coins.
x/poolmanager/protorev.go (3)
46-50: Retrieve burn fees method replicates existing patterns.This new getter mirrors the approach used for community pool and stakers. Ensure that any routes calling this method handle empty slices gracefully.
52-56: Getter by denom method is consistent with stakers/community versions.Returning a zero coin when not found is a sound approach. Confirm all error callers handle the returned
nilerror correctly.
58-61: Fee update method looks correct.The function properly increases coin amounts for burning. Double-check any inbound negative or zero values in upstream calls to avoid unexpected increases.
x/txfees/keeper/hooks.go (2)
106-129: Burning OSMO taker fees logic is coherent.The added check for
osmoTakerFeeDistribution.Burnand subsequent calculation is straightforward. Sending coins to a fixed burn address is a valid approach for chain-based burns. However, consider verifying the reliability of this "null address" (osmo1qqq...mcn030) in tests or mainnet docs.
239-245: Isolation of non-native, non-whitelisted burn amounts is clear.Sending coins to
TakerFeeBurnNamefor subsequent swapping is a clean pattern. Check that the associated account is set with the correct permissions to avoid accidental interactions.
| // KeyTakerFeeBurnProtoRevArray defines key to store the taker fee for burn tracker coin array. | ||
| KeyTakerFeeBurnProtoRevArray = []byte{0x0B} |
There was a problem hiding this comment.
Potential key collision detected.
The new KeyTakerFeeBurnProtoRevArray is using byte value 0x0B, but this same value is already used for KeyTakerFeeShare defined on line 59. This could cause key collisions in storage, potentially leading to data corruption or unexpected behavior.
Consider using a different byte value that is not currently in use, such as 0x0D or higher.
|
Thanks for this PR @VolodymyrBg, great work. We would prefer this work be done in house with proper discussion and designs. I am closing this for now, but this PR will be referenced in the issue. |
What is the purpose of the change
This PR implements a new burn mechanism for taker fees as requested by the community. Currently, taker fees are distributed between staking rewards and the community pool. This change adds a third option to burn a portion of the taker fees by sending them to a null address, providing a more significant burn mechanism than the existing ProtoRev feature.
Closes #8921
Key changes:
Testing and Verifying
This change can be verified as follows:
Documentation and Release Note
Where is the change documented?