Skip to content

SOV-4106 Feat: Fee sharing collector with direct transfer #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cwsnt wants to merge 5 commits into
base: bobDevelopment
Choose a base branch
from
Open

SOV-4106 Feat: Fee sharing collector with direct transfer #2

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce99bf0
feat: support fee sharing collector
May 24, 2024
9f4da78
init: convertion lib dex
Jun 4, 2024
187c965
add readme for on going lib
Jun 4, 2024
97b3b40
init: collect protocol fee with direct transfer
Jun 12, 2024
aa9b3d4
1/ address comment
Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions contracts/callpaths/ColdPath.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ contract ColdPath is MarketSequencer, DepositDesk, ProtocolAccount {
using Chaining for Chaining.PairFlow;
using ProtocolCmd for bytes;

/* @dev access control for treasury role */
modifier onlyTreasury() {
require(msg.sender == treasury_, "Only Treasury");
_;
}

/* @notice Consolidated method for protocol control related commands. */
function protocolCmd (bytes calldata cmd) virtual public {
uint8 code = uint8(cmd[31]);
Expand Down Expand Up @@ -71,9 +77,7 @@ contract ColdPath is MarketSequencer, DepositDesk, ProtocolAccount {
require(sudoMode_, "Sudo");
uint8 cmdCode = uint8(cmd[31]);

if (cmdCode == ProtocolCmd.COLLECT_TREASURY_CODE) {
collectProtocol(cmd);
} else if (cmdCode == ProtocolCmd.SET_TREASURY_CODE) {
if (cmdCode == ProtocolCmd.SET_TREASURY_CODE) {
setTreasury(cmd);
} else if (cmdCode == ProtocolCmd.AUTHORITY_TRANSFER_CODE) {
transferAuthority(cmd);
Expand Down Expand Up @@ -110,6 +114,8 @@ contract ColdPath is MarketSequencer, DepositDesk, ProtocolAccount {
resetNonceCond(cmd);
} else if (cmdCode == UserCmd.GATE_ORACLE_COND) {
checkGateOracle(cmd);
} else if (cmdCode == UserCmd.COLLECT_TREASURY_CODE) {
collectProtocol(cmd);
} else {
revert("Invalid command");
}
Expand Down Expand Up @@ -242,7 +248,7 @@ contract ColdPath is MarketSequencer, DepositDesk, ProtocolAccount {
/* @notice Pays out the the protocol fees.
* @param token The token for which the accumulated fees are being paid out.
* (Or if 0x0 pays out native Ethereum.) */
function collectProtocol (bytes calldata cmd) private {
function collectProtocol (bytes calldata cmd) private onlyTreasury {
(, address token) = abi.decode(cmd, (uint8, address));

require(block.timestamp >= treasuryStartTime_, "Treasury start");
Expand All @@ -257,7 +263,6 @@ contract ColdPath is MarketSequencer, DepositDesk, ProtocolAccount {

require(treasury != address(0) && treasury.code.length != 0, "Treasury invalid");
treasury_ = treasury;
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to keep it universal i suggest to restore the original code

treasuryStartTime_ = uint64(block.timestamp + 7 days);

but replace 7 days with a constant TREASURY_START_TIME_OFFSET to have this option if needed on other chains.
The BOB and default implementation should have it set to 0.

treasuryStartTime_ = uint64(block.timestamp + 7 days);
emit SdexEvents.TreasurySet(treasury_, treasuryStartTime_);
}

Expand Down
1 change: 1 addition & 0 deletions contracts/callpaths/LongPath.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import '../libraries/Directives.sol';
import '../libraries/Encoding.sol';
import '../libraries/TokenFlow.sol';
import '../libraries/PriceGrid.sol';
import '../libraries/ProtocolCmd.sol';
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need it here?

import '../mixins/MarketSequencer.sol';
import '../mixins/SettleLayer.sol';
import '../mixins/PoolRegistry.sol';
Expand Down
8 changes: 8 additions & 0 deletions contracts/interfaces/IFeeProtocolCollector.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity 0.8.19;

/// @title Minimal ERC20 interface for Uniswap
/// @notice Contains a subset of the full ERC20 interface that is used in Uniswap V3
interface IFeeProtocolCollector {
function transferTokens(address _token, uint96 _amount) external;
}
8 changes: 8 additions & 0 deletions contracts/interfaces/ISdexQuery.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity 0.8.19;

import "../libraries/PoolSpecs.sol";

interface ISdexQuery {
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this interface is not used anywhere

function queryPoolParams (address base, address quote, uint256 poolIdx) external view returns (PoolSpecs.Pool memory pool);
}
9 changes: 7 additions & 2 deletions contracts/libraries/ProtocolCmd.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ library ProtocolCmd {
uint8 constant HOT_OPEN_CODE = 22;
// Code to toggle on or off emergency safe mode
uint8 constant SAFE_MODE_CODE = 23;
// Code to collect accumulated protocol fees for the treasury.
uint8 constant COLLECT_TREASURY_CODE = 40;
// Code to set the protocol treasury
uint8 constant SET_TREASURY_CODE = 41;
////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -113,4 +111,11 @@ library UserCmd {
uint8 constant BURN_KNOCKOUT = 92;
uint8 constant CLAIM_KNOCKOUT = 93;
uint8 constant RECOVER_KNOCKOUT = 94;


////////////////////////////////////////////////////////////////////////////
// Protocol Fee command codes
////////////////////////////////////////////////////////////////////////////
// Code to collect accumulated protocol fees for the treasury.
uint8 constant COLLECT_TREASURY_CODE = 40;
}
3 changes: 3 additions & 0 deletions contracts/libraries/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# SdexConvertLib
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no SdexConvertLib
do you want to keep this README.md?

This SdexConvertLib is meant to be used for inside & outside integration to do the swap to this Sdex protocol (which support both direct & multihop swap)
This libraray is not 100% ready to be used, since it's still in progress and not fully tested & covered by unit tests.
11 changes: 10 additions & 1 deletion contracts/mixins/ProtocolAccount.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import '../libraries/TokenFlow.sol';
import '../libraries/SafeCast.sol';
import './StorageLayout.sol';

import '../interfaces/IFeeProtocolCollector.sol';

/* @title Protocol Account Mixin
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ProtocolAccount contract is inherited in WarmPath, but seems not to be used there.
pls double check and remove the inheritance.
the contract doesn't have it own storage, so should be safe.

* @notice Tracks and pays out the accumulated protocol fees across the entire exchange
* These are the fees belonging to the SdexSwap protocol, not the liquidity
Expand Down Expand Up @@ -36,13 +38,20 @@ contract ProtocolAccount is StorageLayout {

/* @notice Pays out the earned, but unclaimed protocol fees in the pool.
* @param recv - The receiver of the protocol fees.
* @param token - The token address of the quote token. */
* @param token - The token address of the quote token.
* @return withdrawn tokens */
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in doesn't return anything

function disburseProtocolFees (address recv, address token) internal {
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need a receiver (recv) here. also see comments below for clarity.
tokens should be an array

Suggested change
function disburseProtocolFees (address recv, address token) internal {
function disburseProtocolFees (address[] tokens) internal {

uint128 collected = feesAccum_[token];
feesAccum_[token] = 0;
if (collected > 0) {
/**
* directly deposit token to fee protocol collector
*/
bytes32 payoutKey = keccak256(abi.encode(recv, token));
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a constant PROTOCOL_FEES_RECEIVER_HASH - hash of 'PROTOCOL_FEES_RECEIVER' + some nonce - can be a hardcoded hash and use instead of recv because should we replace the FeeSharingCollector, the fees collected should be paid out to the new FSC, otherwise this portion will stuck or we would need to call from the previous FSC which is tricky - need not to forget to do it. once replaced - we should only transfer all the fees to he new FSC - one time operation.

Suggested change
bytes32 payoutKey = keccak256(abi.encode(recv, token));
bytes32 payoutKey = keccak256(abi.encode(PROTOCOL_FEES_RECEIVER_HASH, token));

userBals_[payoutKey].surplusCollateral_ += collected;
require(userBals_[payoutKey].surplusCollateral_ <= type(uint96).max, "Value exceeds uint96 range");
Copy link
Copy Markdown

@koirikivi koirikivi Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really related to this change, but uint96 can theoretically overflow with something like POWA (where the total potential supply is 1 000 000 000 000 tokens with 18 decimals, or 1e30. uint96 max value is only around ~7.9e28)

Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch
this is the FeeSharingCollector contract's limit.
on the one hand - yes, it is technically possible to collect 1B, but in fact the fees are much less, practical max we had is 0.5%, so it is reduced down to 5e27 and only in case when the total supply is swapped which i doubt is practical.
i think we should restrict tokens with more than 18 deciamls and total supply <= 1B (trillion) for pools creation.
in case we ever reach the limit, we will upgrade (replace) the fee sharing collector.

IFeeProtocolCollector(treasury_).transferTokens(token, uint96(userBals_[payoutKey].surplusCollateral_));
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. FeeSharingCollector requires tokens approval for ERC20 tokens which is missing here.
    1.2 For the native token the function should be called with value payload.
  2. pls add tests - would have helped to discover it.

userBals_[payoutKey].surplusCollateral_ = 0;
}
}
}
5 changes: 5 additions & 0 deletions contracts/test/MockTimelock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
pragma solidity 0.8.19;

import "../governance/SdexPolicy.sol";
import "../SdexSwapDex.sol";
import "hardhat/console.sol";

contract MockTimelock {
Expand Down Expand Up @@ -47,4 +48,8 @@ contract MockTimelock {
return SdexPolicy(policy_).setPolicy(conduit, proxyPath, policy);
}

function userCmd (address minion, uint16 proxyPath,
bytes calldata cmd) public {
SdexSwapDex(minion).userCmd(proxyPath, cmd);
}
}
37 changes: 16 additions & 21 deletions test/TestPool.govern.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,44 +148,39 @@ describe('Pool Governance', () => {
})


it("collect treasury", async() => {
it("successfully collect treasury without time delay", async() => {
await test.testRevisePool(feeRate, 128, 1) // Turn on protocol fee
await pool.connect(await test.auth).protocolCmd(test.COLD_PROXY, transferCmd(policy.address), true)
await pool.connect(await test.auth).protocolCmd(test.COLD_PROXY, transferCmd(policy2.address), true)
Copy link
Copy Markdown
Contributor

@tjcloa tjcloa Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is policy replaced with policy2 here?

await test.testMintAmbient(10000)
await test.testSwap(true, false, 100000, MAX_PRICE)

await treasury.treasuryResolution(pool.address, test.COLD_PROXY, treasurySetCmd(policy2.address), true)
await hre.ethers.provider.send("evm_increaseTime", [3600*24*7+1]) // 7 days
await treasury2.treasuryResolution(pool.address, test.COLD_PROXY, treasurySetCmd(treasury2.address), true)

// Unauthorized attempts to collect treasury
await expect(pool.protocolCmd(test.COLD_PROXY, collectCmd(), true)).to.be.reverted
await expect(ops.opsResolution(pool.address, test.COLD_PROXY, collectCmd())).to.be.reverted
await expect(ops.treasuryResolution(pool.address, test.COLD_PROXY, collectCmd(), true)).to.be.reverted
await expect(treasury.userCmd(pool.address, test.COLD_PROXY, collectCmd())).to.be.revertedWith("Only Treasury")

// Successful treasury payout
let snap = await (await test.query).querySurplus(policy2.address, baseToken.address)
await treasury.treasuryResolution(pool.address, test.COLD_PROXY, collectCmd(), true)
expect(await (await test.query).querySurplus(policy2.address, baseToken.address)).to.gt(snap);
let snap = await (await test.query).querySurplus(treasury2.address, baseToken.address)
await treasury2.userCmd(pool.address, test.COLD_PROXY, collectCmd())
expect(await (await test.query).querySurplus(treasury2.address, baseToken.address)).to.gt(snap);
})

it("collect treasury time delay", async() => {
it("successfully collect treasury with time delay", async() => {
await test.testRevisePool(feeRate, 128, 1) // Turn on protocol fee
await pool.connect(await test.auth).protocolCmd(test.COLD_PROXY, transferCmd(policy.address), true)
await pool.connect(await test.auth).protocolCmd(test.COLD_PROXY, transferCmd(policy2.address), true)
await test.testMintAmbient(10000)
await test.testSwap(true, false, 100000, MAX_PRICE)

await treasury.treasuryResolution(pool.address, test.COLD_PROXY, treasurySetCmd(policy2.address), true)
await treasury2.treasuryResolution(pool.address, test.COLD_PROXY, treasurySetCmd(treasury2.address), true)

// Will fail because treasury can only be collected 7 days after treasury address is set
await expect(treasury.treasuryResolution(pool.address, test.COLD_PROXY, collectCmd(), true)).to.be.reverted
await hre.ethers.provider.send("evm_increaseTime", [3600*24*6]) // 6 days
await expect(treasury.treasuryResolution(pool.address, test.COLD_PROXY, collectCmd(), true)).to.be.reverted
await hre.ethers.provider.send("evm_increaseTime", [3600*24+1]) // One more day... treasury valid
// Unauthorized attempts to collect treasury
await expect(treasury.userCmd(pool.address, test.COLD_PROXY, collectCmd())).to.be.revertedWith("Only Treasury")

await hre.ethers.provider.send("evm_increaseTime", [3600*24*6]) // 6 days
// Successful treasury payout
let snap = await (await test.query).querySurplus(policy2.address, baseToken.address)
await treasury.treasuryResolution(pool.address, test.COLD_PROXY, collectCmd(), true)
expect(await (await test.query).querySurplus(policy2.address, baseToken.address)).to.gt(snap);
let snap = await (await test.query).querySurplus(treasury2.address, baseToken.address)
await treasury2.userCmd(pool.address, test.COLD_PROXY, collectCmd())
expect(await (await test.query).querySurplus(treasury2.address, baseToken.address)).to.gt(snap);
})


Expand Down