bancorprotocol / carbon-contracts Goto Github PK

GOALS:

Primary goal: simplify logic and reduce code complexity.
Secondary goal 1: reduce contract size (already close to the maximum limit).
Secondary goal 2: reduce runtime gas cost for strategy creation and for trading.

TLDR:

Creating a new pair can be reduced to be an internal process, executed only when creating a new strategy (and only when it doesn't already exist, so there shouldn't be any need to revert if it does already exist).

DETAILS:

Function _pairExists sorts the input tokens and reads the corresponding pair ID from storage.

This function is called from:

• Function _createPair, which sorts the input tokens yet again
• Function _pair, which sorts the input tokens AND reads the corresponding pair ID from storage yet again

We can get rid of this redundancy by:

• Defining function _createPair as "create pair if does not exist"
• Allowing function _pair to return a non-existing pair

Doing so allows to:

• Get rid of the PairDoesNotExist / PairAlreadyExists logic and all the related tests
• Reduce contract size by ~300 bytes
• Reduce strategy creation gas cost by ~700
• Reduce trading gas cost by ~700

Note that:

• Function _createPair is called from:
  • Function createPair
  • Function createStrategy
• Function _pair is called from:
  • Function pair
  • Function strategiesByPair
  • Function strategiesByPairCount
  • Function tradeBySourceAmount
  • Function tradeByTargetAmount
  • Function calculateTradeSourceAmount
  • Function calculateTradeTargetAmount

Contract CarbonController:

1. Get rid of function createPair (also in ICarbonController)

2. Replace:

        Pair memory strategyPair;
        if (!_pairExists(token0, token1)) {
            strategyPair = _createPair(token0, token1);
        } else {
            strategyPair = _pair(token0, token1);
        }

With:

        Pair memory strategyPair = _createPair(token0, token1);

Contract Pairs:

1. Get rid of all occurrences (usage and implementation) of function _pairExists

2. Change function _createPair to:

        // sort tokens
        Token[2] memory sortedTokens = _sortTokens(token0, token1);
        uint128 id = _pairIds[sortedTokens[0]][sortedTokens[1]];

        if (id == 0) {
            // increment pair id
            id = _lastPairId + 1;
            _lastPairId = id;

            // store pair
            _pairsStorage[id] = sortedTokens;
            _pairIds[sortedTokens[0]][sortedTokens[1]] = id;

            emit PairCreated(id, sortedTokens[0], sortedTokens[1]);
        }

        return Pair({ id: id, tokens: sortedTokens });

Voucher and CarbonController are missing their deploy tests

New solidity compiler supports it so should be cleaner.

This struct is used only internally in library MathEx (i.e., it is not exposed to other libraries or contracts like in V3).

Replacing it with a pair of uint256 variables saves approximately 500 gas per order per trade.

User input trade actions are verified in two different places in the code:

• The amounts are verified in CarbonController, and the error thrown upon failure is InvalidTradeActionAmount.
• The strategy IDs are verified in Strategies, and the error thrown upon failure is TokensMismatch.

Semantically, it makes more sense to have both checks in the same place.

Practically:

• Placing them in CarbonController increases the gas cost per order
• Placing them in Strategies decreases the gas cost per order (by ~150, but still)

While we're at it, we might also want to rename TokensMismatch to InvalidTradeActionStrategyId.

The only required method in the voucher is tokenOfOwnerByIndex. This significantly reduce creation gas costs.
Optionally the contract can also support returning a list of ids per owner in a single call.

There's currently no way for the admin to withdraw the accumulated fees.
It should include the balance and a target.

Primary goal: allow pausing a strategy without "ddos by trading" (as opposed to updating a strategy).
Secondary goal: allow pausing a strategy a lower gas cost (vs doing the same by updating a strategy).

Contract CarbonController:

    /**
     * @inheritdoc ICarbonController
     */
    function pauseStrategy(uint256 strategyId) external nonReentrant whenNotPaused onlyProxyDelegate {
        // only the owner of the strategy is allowed to pausing it
        if (msg.sender != _voucher.ownerOf(strategyId)) {
            revert AccessDenied();
        }

        // pause strategy
        _pauseStrategy(strategyId);
    }

Contract Strategies:

    /**
     * @dev pauses an existing strategy
     */
    function _pauseStrategy(uint256 strategyId) internal {
        // prepare storage variable
        uint256[3] storage packedOrders = _packedOrdersByStrategyId[strategyId];
        uint256[3] memory packedOrdersMemory = _packedOrdersByStrategyId[strategyId];
        (Order[2] memory orders, bool ordersInverted) = _unpackOrders(packedOrdersMemory);

        // pause
        for (uint256 i = 0; i < 2; i = uncheckedInc(i)) {
            orders[i].A = orders[i].B = 0;
        }

        // store new values if necessary
        uint256[3] memory newPackedOrders = _packOrders(orders, ordersInverted);
        for (uint256 n = 0; n < 3; n = uncheckedInc(n)) {
            if (packedOrdersMemory[n] != newPackedOrders[n]) {
                packedOrders[n] = newPackedOrders[n];
            }
        }

        // emit event
        emit StrategyPaused({ id: strategyId });
    }

Error messages thrown upon incorrect user input trade actions:

• For the amount of a trade action: InvalidTradeActionAmount
• For the strategy ID of a trade action: TokensMismatch

Need to rename TokensMismatch to InvalidTradeActionStrategyId.

It's currently a bit more challenging to differentiate between an owner update and a trade.

The event can be split into 2 -

• StrategyEdited - when a strategy is edited by the owner, and in this case we can also re-add the owner to the event args, so it's more consistent with created/deleted
• StrategyTraded (TBD) - when a strategy is traded through, identical to the existing event

Hello,

I'm encountering an issue when running the yarn test:deploy command in my project. Below is the error message I receive:

yarn test:deploy
yarn run v1.22.21
$ pnpm deploy:prepare:fork && TEST_FORK=1 ./deployments/run-fork.sh HARDHAT_NETWORK=tenderly mocha --require hardhat/register --extension ts --recursive --exit --timeout 600000 --bail --no-exit 'deploy/tests/**/*.ts'

@bancor/[email protected] deploy:prepare:fork /home/user/carbon-contracts
rm -rf deployments/tenderly && cp -rf deployments/mainnet/. deployments/tenderly

Created a fork ID_TENDERLY_CENSOR at [PROJECT NAME]...

Running:

TENDERLY_FORK_ID=ID_TENDERLY_CENSOR HARDHAT_NETWORK=tenderly mocha --require hardhat/register --extension ts --recursive --exit --timeout 600000 --bail --no-exit deploy/tests/**/*.ts

...

1 failing

0100-revoke-roles
"before each" hook for "should revoke deployer roles":
ERROR processing /home/user/carbon-contracts/deploy/scripts/0100-revoke-roles.ts:
TypeError: hardhat_1.ethers.getContract is not a function
at execute (/home/user/carbon-contracts/utils/Deploy.ts:400:35)
at setRole (/home/user/carbon-contracts/utils/Deploy.ts:463:19)
at grantRole (/home/user/carbon-contracts/utils/Deploy.ts:471:59)
at Object.func (/home/user/carbon-contracts/deploy/scripts/0100-revoke-roles.ts:10:20)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
at async DeploymentsManager.executeDeployScripts (/home/user/carbon-contracts/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1226:22)
at async DeploymentsManager.runDeploy (/home/user/carbon-contracts/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1059:5)
Error: ERROR processing /home/user/carbon-contracts/deploy/scripts/0100-revoke-roles.ts:
TypeError: hardhat_1.ethers.getContract is not a function
at execute (utils/Deploy.ts:400:35)
at setRole (utils/Deploy.ts:463:19)
at grantRole (utils/Deploy.ts:471:59)
at Object.func (deploy/scripts/0100-revoke-roles.ts:10:20)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
at async DeploymentsManager.executeDeployScripts (node_modules/hardhat-deploy/src/DeploymentsManager.ts:1226:22)
at async DeploymentsManager.runDeploy (node_modules/hardhat-deploy/src/DeploymentsManager.ts:1059:5)
at DeploymentsManager.executeDeployScripts (node_modules/hardhat-deploy/src/DeploymentsManager.ts:1229:19)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
at async DeploymentsManager.runDeploy (node_modules/hardhat-deploy/src/DeploymentsManager.ts:1059:5)

Deleting a fork ID_TENDERLY_CENSOR from [PROJECT NAME]...

error Command failed with exit code 1.

Additionally, I am encountering a version error when running yarn install. Here is the output I receive:

yarn install v1.22.21
warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
[1/4] Resolving packages...
warning Lockfile has incorrect entry for "[email protected]". Ignoring it.
Couldn't find any versions for "parser" that matches "0.16"
? Please choose a version of "parser" from this list:
❯ 0.1.4
0.1.3
0.1.2
0.1.1
0.1.0

As a workaround, I have to use npm install --force instead of yarn install. I am running Ubuntu 22.04, and I have noticed that yarn test and yarn test:nightly commands work fine.

Any assistance or guidance on resolving these issues would be greatly appreciated.

Thank you.

In library MathEx, implement:

    /**
    * @dev returns the smallest integer `z` such that `x * y / z <= 2 ^ 256 - 1`
    */
    function minFactor(uint256 x, uint256 y) internal pure returns (uint256) {
        Uint512 memory xy = _mul512(x, y);
        unchecked {
            // safe because:
            // - if `x < 2 ^ 256 - 1` or `y < 2 ^ 256 - 1`
            //   then `xy.hi < 2 ^ 256 - 2`
            //   hence neither `xy.hi + 1` nor `xy.hi + 2` overflows
            // - if `x = 2 ^ 256 - 1` and `y = 2 ^ 256 - 1`
            //   then `xy.hi = 2 ^ 256 - 2 = ~xy.lo`
            //   hence `xy.hi + 1`, which does not overflow, is computed
            return xy.hi > ~xy.lo ? xy.hi + 2 : xy.hi + 1;
        }
    }

In contract Strategies, replace every occurrence of:

MathEx.mulDivC(x, y, type(uint256).max)

With:

MathEx.minFactor(x, y)

This should reduce trading cost by ~500 gas per order.

Extending the tests accordingly:

1. In contract TestMathEx, implement:

    function minFactor(uint256 x, uint256 y) external pure returns (uint256) {
        return MathEx.minFactor(x, y);
    }

2. In file MathEx.ts in function testMulDiv, add:

        const tuples = [[x, y], [x, z], [y, z], [x, y.add(z)], [x.add(z), y], [x.add(z), y.add(z)]];
        const values = tuples.filter((tuple) => tuple.every((value) => value.lte(MAX_UINT256)));
        for (const [x, y] of values) {
            it(`minFactor(${x}, ${y})`, async () => {
                const actual = await mathContract.minFactor(x, y);
                expect(mulDivFuncs.mulDivC(x, y, actual)).to.be.lte(MAX_UINT256);
                if (actual.gt(1)) {
                    expect(mulDivFuncs.mulDivC(x, y, actual.sub(1))).to.be.gt(MAX_UINT256);
                }
            });
        }

Is there a doc somewhere with some estimated gas costs for trades and deploying new strategies?

Currently, running yarn test requires tenderly to be set up and logged in.
Running tests is performed on a local network, so there shouldn't be a need for setting up tenderly.
The task is to configure yarn test to work without this extra setup step.