Pool booster #2400

sparrowDom · 2025-02-14T19:14:59Z

Pool booster

This deploys a Pool Booster Factory on Sonic which has the ability to create individual pool booster. Pool booster is a contract that receives forwarded yield (from an AMM pool) and forwards it to corresponding Bribe contract -> which is part of the Gauge incentives.

Notice: PoolBoosterSwapxPair is a pool booster that supports Classic stable and classic volatile pools. Because there are no such pools on SwapX we can not fork tests its functionality. Also we can not test it on other non OS gauges since they don't accept the OS as an incentive token.

Code Change Checklist

To be completed before internal review begins:

The contract code is complete
Executable deployment file
Fork tests that test after the deployment file runs
Unit tests *if needed
The owner has done a full checklist review of the code + tests

Internal review:

Two approvals by internal reviewers

github-actions · 2025-02-14T19:15:32Z

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 4219ada

codecov · 2025-02-14T19:21:24Z

Codecov Report

Attention: Patch coverage is 56.52174% with 30 lines in your changes missing coverage. Please review.

Project coverage is 48.67%. Comparing base (e766fbb) to head (a1ff564).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
...racts/contracts/poolBooster/PoolBoosterFactory.sol	52.17%	22 Missing ⚠️
...cts/contracts/poolBooster/PoolBoosterSwapxPair.sol	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2400      +/-   ##
==========================================
- Coverage   50.96%   48.67%   -2.29%     
==========================================
  Files          92       97       +5     
  Lines        4529     4783     +254     
  Branches     1203     1262      +59     
==========================================
+ Hits         2308     2328      +20     
- Misses       2217     2451     +234     
  Partials        4        4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

clement-ux

I did the first pass on Factory and both PB implementation.

With more hindsight, I'm wondering if this Factory contract is really necessary? Maybe a simple UI can be enough?

Because here, the factory is used to keep a track of all PB deployed, in order to bribes all the PB at the same time. But:

bribesAll() have a significant amount of chance of reverting when amount of registered PB is high.
A simple multicall can do the same, with even more granularity (excluding some PB for example).
as creation of PB is permissioned, factory isn't really needed imo.

I know we already talked about this in the Spec document on Notion, but with more hindsight, I'm less convinced in the need of a Factory.
What's more, every time we will need to add a need PB type, we will need to upgrade the proxy, which is slow (governance) and unsafe! In comparison, adding it in the UI, is way faster, and doesn't imply security matter.

I think having a Factory doesn't add value and bring a lot of hurdles. (Really sorry for changing my mind after finalizing specs).

contracts/contracts/poolBooster/PoolBoosterFactory.sol

contracts/contracts/poolBooster/PoolBoosterSwapxPair.sol

contracts/contracts/poolBooster/PoolBoosterFactory.sol

contracts/contracts/poolBooster/PoolBoosterSwapxIchi.sol

clement-ux

Everything looks really good!
Even though there are more contracts now, I believe it will be easier and faster to scale thanks to this architecture!

I’ve left some minor comments to check out.

contracts/contracts/poolBooster/AbstractPoolBoosterFactory.sol

clement-ux · 2025-02-18T21:48:45Z

contracts/contracts/poolBooster/AbstractPoolBoosterFactory.sol

+        for (uint256 i = 0; i < length; i++) {
+            address poolBoosterAddress = poolBoosters[i].boosterAddress;
+            bool skipBribeCall = false;
+            for (uint256 j = 0; j < _exclusionList.length; j++) {


nit: as we cached length on the previous loop, maybe we can cache it here too? (This is really nit-picking, totally fine to keep it like this).

good point: 77c21d8

clement-ux · 2025-02-18T21:50:06Z

contracts/contracts/poolBooster/AbstractPoolBoosterFactory.sol

+        require(
+            _address.code.length > 0 && _address != address(0),
+            "Failed creating a pool booster"
+        );


This is really good like this I think! 👍

clement-ux · 2025-02-18T21:59:23Z

contracts/contracts/poolBooster/PoolBoostCentralRegistry.sol

+     */
+    function removeFactory(address _factoryAddress) external onlyGovernor {
+        require(_factoryAddress != address(0), "Invalid address");
+        require(isApprovedFactory(_factoryAddress), "Not an approved factory");


I think we don't need this check. It is doing 2 time the same thing.
First we go through all factories to ensure it is there, then we go through all factories a second time and then if this is there, remove it.

Maybe isApprovedFactory can return the position in the array to avoid doing 2 times the loop?

Maybe we can just remove this check?

However, gas is cheap and too much check isn't an issue, so it is ok to keep it if you like.

This is a governance action an not really expected to be called often (if at all). For that reason I wouldn't be too worried with extra gas usage?

clement-ux · 2025-02-18T22:01:18Z

contracts/contracts/poolBooster/PoolBoosterSwapxIchi.sol

+
+        uint256 osBribeAmount = balance.mulTruncate(split);
+        // -1 to prevent possible rounding issues with OS token
+        uint256 otherBribeAmount = balance - osBribeAmount - 1;


I have fuzz tested that balance could never be equal to osBribeAmount (due to rounding issue in mulTruncate). 👍

thanks for that

clement-ux · 2025-02-18T22:04:07Z

contracts/contracts/poolBooster/PoolBoostCentralRegistry.sol

+    function approveFactory(address _factoryAddress) external onlyGovernor {
+        require(_factoryAddress != address(0), "Invalid address");
+
+        factories.push(_factoryAddress);
+        emit FactoryApproved(_factoryAddress);
+    }


There is no check to ensure that the factory isn't already approved.
But in the other hand, I don't see any issue if we have 2 times the same factory here.
So I'm fine to keep it like this 👍

Actually, that is a great point. Adding a check and a test here: 735f55f

In case we have 2 entries of the same factory we could get a false sense of security from calling remove once. Since the other factory would still be in the list and considered supported.

clement-ux · 2025-02-18T22:07:47Z

contracts/contracts/poolBooster/PoolBoosterFactorySwapxPair.sol

+import { PoolBoosterSwapxPair } from "./PoolBoosterSwapxPair.sol";
+import { AbstractPoolBoosterFactory } from "./AbstractPoolBoosterFactory.sol";
+
+import "hardhat/console.sol";


713eab5 😬

pandadefi · 2025-02-19T11:49:50Z

contracts/contracts/poolBooster/PoolBoosterSwapxIchi.sol

+            "Invalid bribeContractOther address"
+        );
+        // expect it to be between 1% & 99%
+        require(_split > 1e16 && _split < 99e16, "Unexpected split amount");


range could be between 0 and 100%.

pandadefi · 2025-02-19T12:18:09Z

contracts/contracts/poolBooster/PoolBoosterSwapxIchi.sol

+    constructor(
+        address _bribeContractOS,
+        address _bribeContractOther,
+        address _osToken,


The osToken should be oeth, ousd... tokens with 18 decimals. Maybe just make sure it's not a token with less decimals that is added to make sure MIN_BRIBE_AMOUNT is still valid

pandadefi · 2025-02-19T12:23:37Z

contracts/contracts/poolBooster/PoolBoosterFactorySwapxPair.sol

+            "Invalid ammPoolAddress address"
+        );
+        require(_salt > 0, "Invalid salt");
+


The swapX gauges can't be created by anyone, how will the deployment workflow be like?

sparrowDom added 4 commits February 12, 2025 22:48

initial pool booster commit

014e606

add initial test

38313ae

more development for the pool booster

1f4917b

add more tests

a1ff564

sparrowDom added 2 commits February 14, 2025 22:10

add final tests

e069aef

prettier

13a16cc

sparrowDom marked this pull request as ready for review February 14, 2025 21:11

sparrowDom requested review from DanielVF, naddison36, shahthepro and clement-ux as code owners February 14, 2025 21:11

clement-ux reviewed Feb 16, 2025

View reviewed changes

sparrowDom added 11 commits February 17, 2025 10:42

fix the issue with salt

fe6005d

refactor code

c6b01f3

simlify removal of pool booster entry

0d7c392

do not execute bribes when there are too little funds to bribe with

4297827

move require checks to pool boosters

5436d54

remove unneeded code

6aaa8c9

add the ability to exclude pool boosters when calling bribeAll

1fb2b10

refactor pool booster factory into an immutable one

9ded3d2

add svg doc files

98c22c7

add a central Factory Registry and tests for it

4562798

add another test

16c6807

clement-ux reviewed Feb 18, 2025

View reviewed changes

sparrowDom added 4 commits February 19, 2025 09:38

remove unused code

df5f5cb

remove gap

b39f08f

cache the length

77c21d8

remove hardhat console

713eab5

sparrowDom added 2 commits February 19, 2025 09:49

do not allow double factory approval

735f55f

prettier

4219ada

pandadefi reviewed Feb 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pool booster #2400

Pool booster #2400

sparrowDom commented Feb 14, 2025

github-actions bot commented Feb 14, 2025 •

edited

Loading

codecov bot commented Feb 14, 2025

clement-ux left a comment •

edited

Loading

clement-ux left a comment

clement-ux Feb 18, 2025

sparrowDom Feb 19, 2025

clement-ux Feb 18, 2025

clement-ux Feb 18, 2025

sparrowDom Feb 19, 2025

clement-ux Feb 18, 2025

sparrowDom Feb 19, 2025

clement-ux Feb 18, 2025

sparrowDom Feb 19, 2025

clement-ux Feb 18, 2025

sparrowDom Feb 19, 2025

pandadefi Feb 19, 2025

pandadefi Feb 19, 2025

pandadefi Feb 19, 2025

Pool booster #2400

Are you sure you want to change the base?

Pool booster #2400

Conversation

sparrowDom commented Feb 14, 2025

Pool booster

Code Change Checklist

github-actions bot commented Feb 14, 2025 • edited Loading

codecov bot commented Feb 14, 2025

Codecov Report

clement-ux left a comment • edited Loading

Choose a reason for hiding this comment

clement-ux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 14, 2025 •

edited

Loading

clement-ux left a comment •

edited

Loading