Skip to content
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.

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

Carbon batch router #175

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions components/Contracts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
CarbonController__factory,
CarbonPOL__factory,
CarbonVortex__factory,
CarbonBatcher__factory,
ERC20__factory,
MockBancorNetworkV3__factory,
OptimizedTransparentUpgradeableProxy__factory,
Expand Down Expand Up @@ -34,6 +35,7 @@ const getContracts = (signer?: Signer) => ({
CarbonController: deployOrAttach('CarbonController', CarbonController__factory, signer),
CarbonVortex: deployOrAttach('CarbonVortex', CarbonVortex__factory, signer),
CarbonPOL: deployOrAttach('CarbonPOL', CarbonPOL__factory, signer),
CarbonBatcher: deployOrAttach('CarbonBatcher', CarbonBatcher__factory, signer),
MockBancorNetworkV3: deployOrAttach('MockBancorNetworkV3', MockBancorNetworkV3__factory, signer),
ProxyAdmin: deployOrAttach('ProxyAdmin', ProxyAdmin__factory, signer),
Voucher: deployOrAttach('Voucher', Voucher__factory, signer),
Expand Down
16 changes: 15 additions & 1 deletion contracts/token/Token.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ using {
safeTransfer,
safeTransferFrom,
safeApprove,
forceApprove,
safeIncreaseAllowance,
unsafeTransfer
unsafeTransfer,
toIERC20
} for Token global;

/* solhint-disable func-visibility */
Expand Down Expand Up @@ -135,6 +137,18 @@ function safeApprove(Token token, address spender, uint256 amount) {
toIERC20(token).safeApprove(spender, amount);
}

/**
* @dev force approves a specific amount of the native token/ERC20 token from a specific holder
*
* note that the function does not perform any action if the native token is provided
*/
function forceApprove(Token token, address spender, uint256 amount) {
if (isNative(token)) {
return;
}
toIERC20(token).forceApprove(spender, amount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing new line?

}

/**
* @dev atomically increases the allowance granted to `spender` by the caller.
*
Expand Down
224 changes: 224 additions & 0 deletions contracts/utility/CarbonBatcher.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;

import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
import { IERC721Receiver } from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import { Address } from "@openzeppelin/contracts/utils/Address.sol";

import { ICarbonController } from "../carbon/interfaces/ICarbonController.sol";
import { IVoucher } from "../voucher/interfaces/IVoucher.sol";

import { Upgradeable } from "./Upgradeable.sol";
import { Order } from "../carbon/Strategies.sol";
import { Utils } from "../utility/Utils.sol";
import { Token } from "../token/Token.sol";

struct StrategyData {
Token[2] tokens;
Order[2] orders;
}

/**
* @dev Contract to batch create carbon controller strategies
*/
contract CarbonBatcher is Upgradeable, Utils, ReentrancyGuard, IERC721Receiver {
using Address for address payable;

error InsufficientNativeTokenSent();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this error move to common/shared utils?


ICarbonController private immutable carbonController;
Copy link
Collaborator

Choose a reason for hiding this comment

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

_carbonController?

IVoucher private immutable voucher;
Copy link
Collaborator

Choose a reason for hiding this comment

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

_voucher?


/**
* @dev triggered when tokens have been withdrawn from the carbon batcher
*/
event FundsWithdrawn(Token indexed token, address indexed caller, address indexed target, uint256 amount);

constructor(
ICarbonController _carbonController,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't prefix function arguments with an _, but if you did this in order to resolve the conflict with state variables, you can call rename these arguments to initCarbonController, and initVoucher, for example

IVoucher _voucher
) validAddress(address(_carbonController)) validAddress(address(_voucher)) {
carbonController = _carbonController;
voucher = _voucher;
_disableInitializers();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing new line?

}

/**
* @dev fully initializes the contract and its parents
*/
function initialize() public initializer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can it be external?

__CarbonBatcher_init();
}

// solhint-disable func-name-mixedcase

/**
* @dev initializes the contract and its parents
*/
function __CarbonBatcher_init() internal onlyInitializing {
__Upgradeable_init();
}

/**
* @inheritdoc Upgradeable
*/
function version() public pure virtual override(Upgradeable) returns (uint16) {
return 1;
}

/**
* @notice creates several new strategies, returns the strategies id's
Copy link
Collaborator

Choose a reason for hiding this comment

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

ids?

*
* requirements:
*
* - the caller must have approved the tokens with assigned liquidity in the orders
*/
function batchCreate(
StrategyData[] calldata strategies
) external payable greaterThanZero(strategies.length) nonReentrant returns (uint256[] memory) {
uint256[] memory strategyIds = new uint256[](strategies.length);
uint256 txValueLeft = msg.value;

// extract unique tokens and amounts
(Token[] memory uniqueTokens, uint256[] memory amounts) = _extractUniqueTokensAndAmounts(strategies);
// transfer funds from user for strategies
for (uint256 i = 0; i < uniqueTokens.length; i = uncheckedInc(i)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps it's time to upgrade to Solidity >0.8.22? uncheckedInc(i) will be no longer needed, for example

Token token = uniqueTokens[i];
uint256 amount = amounts[i];
if (token.isNative()) {
if (txValueLeft < amount) {
revert InsufficientNativeTokenSent();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check isn't strictly needed due to the checked subtraction below

}
txValueLeft -= amount;
continue;
}
token.safeTransferFrom(msg.sender, address(this), amount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These can be in an else clause to prevent nop call for ETH

_setCarbonAllowance(token, amount);
}

// create strategies and transfer nfts to user
for (uint256 i = 0; i < strategies.length; i = uncheckedInc(i)) {
// get tokens for this strategy
Token[2] memory tokens = strategies[i].tokens;
Order[2] memory orders = strategies[i].orders;
// if any of the tokens is native, send this value with the create strategy tx
uint256 valueToSend = 0;
if (tokens[0].isNative()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can cache tokens[0]and tokens[1]

valueToSend = orders[0].y;
} else if (tokens[1].isNative()) {
valueToSend = orders[1].y;
}

// create strategy on carbon
strategyIds[i] = carbonController.createStrategy{ value: valueToSend }(tokens[0], tokens[1], orders);
// transfer nft to user
voucher.safeTransferFrom(address(this), msg.sender, strategyIds[i], "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm.. maybe createStrategy can receive an receipient or owner argument and mint the voucher to it directly? This will prevent the additional transfer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can cache strategyIds[i] and assign it only later

}
// refund user any remaining native token
if (txValueLeft > 0) {
// forwards all available gas
payable(msg.sender).sendValue(txValueLeft);
}

return strategyIds;
}

/**
* @notice withdraws funds held by the contract and sends them to an account
*
* requirements:
*
* - the caller is admin of the contract
*/
function withdrawFunds(
Token token,
address payable target,
uint256 amount
) external validAddress(target) onlyAdmin nonReentrant {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this function required?

if (amount == 0) {
return;
}

// forwards all available gas in case of ETH
token.unsafeTransfer(target, amount);

emit FundsWithdrawn({ token: token, caller: msg.sender, target: target, amount: amount });
}

function onERC721Received(address, address, uint256, bytes calldata) external pure returns (bytes4) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing NATSPEC?

return IERC721Receiver.onERC721Received.selector;
}

/**
* @dev extracts unique tokens and amounts for each token from the strategy data
*/
function _extractUniqueTokensAndAmounts(
StrategyData[] calldata strategies
) private pure returns (Token[] memory uniqueTokens, uint256[] memory amounts) {
// Maximum possible unique tokens
Token[] memory tempUniqueTokens = new Token[](strategies.length * 2);
uint256[] memory tempAmounts = new uint256[](strategies.length * 2);
uint256 uniqueCount = 0;

for (uint256 i = 0; i < strategies.length; i = uncheckedInc(i)) {
StrategyData calldata strategy = strategies[i];

for (uint256 j = 0; j < 2; j = uncheckedInc(j)) {
Token token = strategy.tokens[j];
uint128 amount = strategy.orders[j].y;

// Check if the token is already in the uniqueTokens array
uint256 index = _findInArray(token, tempUniqueTokens, uniqueCount);
if (index == type(uint256).max) {
// If not found, add to the array
tempUniqueTokens[uniqueCount] = token;
tempAmounts[uniqueCount] = amount;
uniqueCount++;
} else {
// If found, aggregate the amount
tempAmounts[index] += amount;
}
}
}

// Resize the arrays to fit the unique count
uniqueTokens = new Token[](uniqueCount);
amounts = new uint256[](uniqueCount);

for (uint256 i = 0; i < uniqueCount; i = uncheckedInc(i)) {
uniqueTokens[i] = tempUniqueTokens[i];
amounts[i] = tempAmounts[i];
}

return (uniqueTokens, amounts);
}

function _findInArray(Token element, Token[] memory array, uint256 arrayLength) private pure returns (uint256) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing NATSPEC

for (uint256 i = 0; i < arrayLength; i = uncheckedInc(i)) {
if (array[i] == element) {
return i;
}
}
return type(uint256).max; // Return max value if not found
}

/**
* @dev set carbon controller allowance to 2 ** 256 - 1 if it's less than the input amount
*/
function _setCarbonAllowance(Token token, uint256 inputAmount) private {
if (token.isNative()) {
return;
}
uint256 allowance = token.toIERC20().allowance(address(this), address(carbonController));
if (allowance < inputAmount) {
// increase allowance to the max amount if allowance < inputAmount
token.forceApprove(address(carbonController), type(uint256).max);
}
}

function uncheckedInc(uint256 i) private pure returns (uint256 j) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing NATSPEC

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it moved to a common/shared utilities?

unchecked {
j = i + 1;
}
}
}
20 changes: 20 additions & 0 deletions deploy/scripts/mainnet/0018-CarbonBatcher.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { DeployedContracts, deployProxy, InstanceName, setDeploymentMetadata } from '../../../utils/Deploy';
import { DeployFunction } from 'hardhat-deploy/types';
import { HardhatRuntimeEnvironment } from 'hardhat/types';

const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironment) => {
const { deployer } = await getNamedAccounts();

const carbonController = await DeployedContracts.CarbonController.deployed();
const voucher = await DeployedContracts.Voucher.deployed();

await deployProxy({
name: InstanceName.CarbonBatcher,
from: deployer,
args: [carbonController.address, voucher.address]
});

return true;
};

export default setDeploymentMetadata(__filename, func);
33 changes: 33 additions & 0 deletions deploy/tests/mainnet/0018-carbon-batcher.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { CarbonBatcher, ProxyAdmin } from '../../../components/Contracts';
import { DeployedContracts, describeDeployment } from '../../../utils/Deploy';
import { expect } from 'chai';
import { ethers } from 'hardhat';

describeDeployment(__filename, () => {
let proxyAdmin: ProxyAdmin;
let carbonBatcher: CarbonBatcher;

beforeEach(async () => {
proxyAdmin = await DeployedContracts.ProxyAdmin.deployed();
carbonBatcher = await DeployedContracts.CarbonBatcher.deployed();
});

it('should deploy and configure the carbon batcher contract', async () => {
expect(await proxyAdmin.getProxyAdmin(carbonBatcher.address)).to.equal(proxyAdmin.address);
expect(await carbonBatcher.version()).to.equal(1);
});

it('carbon batcher implementation should be initialized', async () => {
const implementationAddress = await proxyAdmin.getProxyImplementation(carbonBatcher.address);
const carbonBatcherImpl: CarbonBatcher = await ethers.getContractAt('CarbonBatcher', implementationAddress);
// hardcoding gas limit to avoid gas estimation attempts (which get rejected instead of reverted)
const tx = await carbonBatcherImpl.initialize({ gasLimit: 6000000 });
await expect(tx.wait()).to.be.reverted;
});

it('cannot call postUpgrade on carbon batcher', async () => {
// hardcoding gas limit to avoid gas estimation attempts (which get rejected instead of reverted)
const tx = await carbonBatcher.postUpgrade(true, '0x', { gasLimit: 6000000 });
await expect(tx.wait()).to.be.reverted;
});
});
Loading
Loading