feat(amm)!: make the swap fee instance-wide in the AMM config, not pe… - #360
Open
0x-r4bbit wants to merge 1 commit into
Open
feat(amm)!: make the swap fee instance-wide in the AMM config, not pe…#3600x-r4bbit wants to merge 1 commit into
0x-r4bbit wants to merge 1 commit into
Conversation
…r-pool The swap fee was a per-pool value stored in PoolDefinition and chosen at pool creation (restricted to the tiers 1/5/30/100 bps). Move it to the namespace: the fee is now a single AmmConfig.swap_fee_bps, set once by whoever initializes the instance and applied to every swap under it. This is the first half of the fee work in RFP logos-co/rfp#168 (protocol fees follow in a later commit). On-chain: - AmmConfig gains swap_fee_bps; PoolDefinition drops fees. - Initialize takes swap_fee_bps (any value below FEE_BPS_DENOMINATOR = 100%, validated by assert_valid_swap_fee_bps); NewDefinition no longer takes fees. - swap reads config_data.swap_fee_bps; add/remove/sync drop their per-pool assert_supported_fee_tier checks. IDL regenerated. Off-chain: - FFI: the swap-quote and resolve-pool requests take the config account and read the fee from it; create-pool takes no fee; config_account now returns swapFeeBps. - AMM module + app: createPool sends no feeBps; the new-position form drops the fee-tier selector; pool/registry config entries no longer carry feeBps (the app reads the instance fee from the config, surfaced as resolvePoolAccount.feeBps / configAccount.swapFeeBps). RegistryLoader no longer requires feeBps on a pool entry, so fee-free pools still render. - Setup script + docs updated to the config-fee model. BREAKING CHANGE: The AMM swap fee is now instance-wide, not per-pool. * Account layouts change (Borsh): AmmConfig gains `swap_fee_bps`; PoolDefinition drops `fees`. Existing on-chain config and pool accounts are incompatible and must be recreated. * Instruction ABI: `Initialize` requires `swap_fee_bps`; `NewDefinition` no longer accepts `fees`. Regenerate IDL-based clients (artifacts/amm-idl.json is updated). * The fee is any value in `[0, FEE_BPS_DENOMINATOR)` set at `Initialize` — it is no longer restricted to the 1/5/30/100 bps tiers, and there is no per-pool fee. * amm_core API: `PoolDefinition.fees` removed, `AmmConfig.swap_fee_bps` added; `initialize()` gains a `swap_fee_bps` parameter and `new_definition()` drops its `fees` parameter; new `assert_valid_swap_fee_bps` (the tier helpers are now unused on-chain). * AMM module / FFI: `createPool` takes no `feeBps` (errors `bad_fee_bps_amount` / `invalid_fee_tier` removed); swap-quote and resolve-pool FFI requests require the `config` account; `configAccount` returns `swapFeeBps`. `AMM_POOLS_CONFIG` and the registry pool schema no longer include `feeBps`.
0x-r4bbit
requested review from
3esmit and
gravityblast
and
a lite review from Copilot
September 8, 2026 16:10
There was a problem hiding this comment.
🟡 Changes recommended
The registry pool parser now accepts entries missing required on-chain id fields, which can lead to rendered-but-unresolvable pool rows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves AMM swap fees from per-pool configuration to an instance-wide value stored on AmmConfig, updating on-chain instruction ABI/account layouts plus the off-chain FFI and AMM app to source/display the fee from the config.
Changes:
- On-chain: add
AmmConfig.swap_fee_bps, removePoolDefinition.fees, and updateInitialize/NewDefinition/swap logic accordingly (IDL regenerated). - Off-chain: update FFI requests/quotes/pool resolution and C++ module plumbing to pass/read the config for fee sourcing.
- App/scripts/docs: remove fee-tier selection and feeBps pool-config fields; display the instance-wide fee from
configAccount().
File summaries
| File | Description |
|---|---|
| programs/amm/src/update_config.rs | Updates config fixtures/tests for new swap_fee_bps field. |
| programs/amm/src/tests.rs | Adjusts AMM program tests for config-wide fee and removes per-pool fee usage. |
| programs/amm/src/sync.rs | Removes per-pool fee-tier validation from sync path. |
| programs/amm/src/swap.rs | Sources swap fee from AmmConfig.swap_fee_bps instead of pool definition. |
| programs/amm/src/remove.rs | Removes per-pool fee-tier validation from remove-liquidity path. |
| programs/amm/src/new_definition.rs | Removes fees argument and stops persisting per-pool fee. |
| programs/amm/src/initialize.rs | Adds swap_fee_bps parameter and validates it at initialization. |
| programs/amm/src/create_price_observations.rs | Updates fixtures for swap_fee_bps and drops per-pool fee field usage. |
| programs/amm/src/create_oracle_price_account.rs | Updates fixtures for swap_fee_bps and drops per-pool fee field usage. |
| programs/amm/src/add.rs | Removes per-pool fee-tier validation from add-liquidity path. |
| programs/amm/methods/guest/src/bin/amm.rs | Updates guest instruction surface for Initialize/NewDefinition ABI changes. |
| programs/amm/core/src/lib.rs | Adds AmmConfig.swap_fee_bps, removes PoolDefinition.fees, adds validation helper, updates Instruction ABI. |
| modules/amm/src/amm_module_impl.h | Updates module API docs for fee now being config-wide. |
| modules/amm/src/amm_module_impl.cpp | Passes config into resolve/quote FFI calls and removes fee from createPool request. |
| modules/amm/README.md | Updates module documentation to describe config-wide fee behavior. |
| modules/amm/ffi/src/api/token_holdings.rs | Updates test fixture config decoding for swap_fee_bps. |
| modules/amm/ffi/src/api/tests.rs | Updates config fixture and asserts swapFeeBps surfaced from config. |
| modules/amm/ffi/src/api/swap.rs | Reads swap fee from config account read for resolve/quotes and updates tests accordingly. |
| modules/amm/ffi/src/api/request.rs | Adds required config account read to resolve/quote request schemas; removes create-pool fee field. |
| modules/amm/ffi/src/api/liquidity.rs | Removes fee-tier validation and omits fee from NewDefinition instruction encoding. |
| modules/amm/ffi/src/api/config.rs | Surfaces swapFeeBps from decoded config account. |
| artifacts/amm-idl.json | Regenerates IDL to reflect ABI and account layout changes. |
| apps/amm/VALIDATION.md | Updates validation checklist to reflect no fee selection (instance-wide fee). |
| apps/amm/tests/testnet/setup-amm-testnet.sh | Updates setup script to initialize config with swap fee and remove pool fee input. |
| apps/amm/tests/qml/tst_PoolsPage.qml | Updates Pools page tests to read/display fee from config. |
| apps/amm/tests/qml/tst_NewPositionForm.qml | Removes fee-tier-related tests now that fee selection is gone. |
| apps/amm/src/RegistryLoader.cpp | Stops requiring feeBps in pool registry entries. |
| apps/amm/README.md | Updates app documentation for instance-wide fee and revised pools config schema. |
| apps/amm/qml/state/NewPositionFlow.qml | Removes per-pool fee from flow state and create-pool request payload. |
| apps/amm/qml/pages/PoolsPage.qml | Loads instance-wide fee from config and applies it to all rendered pool rows. |
| apps/amm/qml/pages/LiquidityPage.qml | Removes fee-tier fetching/selection from liquidity page. |
| apps/amm/qml/components/liquidity/NewPositionForm.qml | Removes fee-tier UI/state/error handling and fee inclusion in requests. |
| apps/amm/qml/components/liquidity/LiquidityConfirmationSummary.qml | Removes fee row from liquidity confirmation summary. |
| apps/amm/amm-pools.json.example | Updates example pool registry entry to omit feeBps. |
Review details
- Files reviewed: 34/34 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
81
to
85
| const QString tokenB = obj.value(QStringLiteral("tokenB")).toString(); | ||
| const QJsonValue feeBps = obj.value(QStringLiteral("feeBps")); | ||
| if (tokenA.isEmpty() || tokenB.isEmpty() || !feeBps.isDouble()) | ||
| // The swap fee is instance-wide (AMM config), no longer a pool field, so it is | ||
| // not required here. Only the pair + on-chain ids identify the pool. | ||
| if (tokenA.isEmpty() || tokenB.isEmpty()) | ||
| continue; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…r-pool
The swap fee was a per-pool value stored in PoolDefinition and chosen at pool creation (restricted to the tiers 1/5/30/100 bps). Move it to the namespace: the fee is now a single AmmConfig.swap_fee_bps, set once by whoever initializes the instance and applied to every swap under it. This is the first half of the fee work in RFP logos-co/rfp#168 (protocol fees follow in a later commit).
On-chain:
Off-chain:
BREAKING CHANGE: The AMM swap fee is now instance-wide, not per-pool.
swap_fee_bps; PoolDefinition dropsfees. Existing on-chain config and pool accounts are incompatible and must be recreated.Initializerequiresswap_fee_bps;NewDefinitionno longer acceptsfees. Regenerate IDL-based clients (artifacts/amm-idl.json is updated).[0, FEE_BPS_DENOMINATOR)set atInitialize— it is no longer restricted to the 1/5/30/100 bps tiers, and there is no per-pool fee.PoolDefinition.feesremoved,AmmConfig.swap_fee_bpsadded;initialize()gains aswap_fee_bpsparameter andnew_definition()drops itsfeesparameter; newassert_valid_swap_fee_bps(the tier helpers are now unused on-chain).createPooltakes nofeeBps(errorsbad_fee_bps_amount/invalid_fee_tierremoved); swap-quote and resolve-pool FFI requests require theconfigaccount;configAccountreturnsswapFeeBps.AMM_POOLS_CONFIGand the registry pool schema no longer includefeeBps.