Should we revert to a single collateral type? about v1-core HOT 5 CLOSED

Thanks for opening this @RusseII and @Namaskar-1F64F for writing this up. Let's move from multiple collateral types to a single collateral type. Here's an article by Sid from Maple that influenced my thinking here.

from v1-core.

Thanks for opening this @RusseII and @Namaskar-1F64F for writing this up. Let's move from multiple collateral types to a single collateral type. Here's an article by Sid from Maple that influenced my thinking here.

Sounds good. Let me 100% confirm that we can support fractional vaults #76 before removing

from v1-core.

Sounds good. Let me 100% confirm that we can support fractional vaults #76 before removing

@Namaskar-1F64F it's been confirmed that we are good to move to single collateral type. Fractional vaults are just ERC20 representation of a single NFT or a basket of NFTs. See #76 if you're interested in more info.

from v1-core.

The usage of collateral in arrays, which also necessitates the usage of arrays for the ratios affects almost all of the functionality of the contract. It actually touches all functions other than repay! This also means that the testing needs to use maps and forEaches instead of simple single-variable comparisons and such. There's also some added complexity technically when thinking about how the contracts work, both from a person writing code, but also as a person reading the code - be in code reviews or when viewing the contract externally.

The differences seem to stop there - the functions are not changed - we are testing and performing actions in the same way. For example the mint function needs to inquire about the amount of collateral, do some math, and mint some coins. Either way we're doing the work. One is in a loop.

Take convert with single collateral:

if (amountOfBondsToConvert == 0) {
    revert ZeroAmount();
}

burn(amountOfBondsToConvert);
address collateralToken = collateralToken;
uint256 convertibilityRatio = convertibilityRatio;
if (convertibilityRatio > 0) {
    uint256 collateralToSend = (amountOfBondsToConvert *
        convertibilityRatio) / 1 ether;
    // external call reentrancy possibility: the bonds are burnt here already - if there weren't enough bonds to burn, an error is thrown
    IERC20(collateralToken).safeTransfer(
        _msgSender(),
        collateralToSend
    );
    totalCollateral[collateralToken] -= collateralToSend;
    emit Converted(
        _msgSender(),
        collateralToken,
        amountOfBondsToConvert,
        collateralToSend
    );
}

if (amountOfBondsToConvert == 0) {
    revert ZeroAmount();
}

burn(amountOfBondsToConvert);
for (uint256 i = 0; i < collateralTokens.length; i++) { // CHANGED need to loop
    address collateralToken = collateralTokens[i]; // CHANGED assign the current affected token
    uint256 convertibilityRatio = convertibilityRatios[i]; // CHANGED assign the current affected ratio
    if (convertibilityRatio > 0) {
        uint256 collateralToSend = (amountOfBondsToConvert *
            convertibilityRatio) / 1 ether;
        // external call reentrancy possibility: the bonds are burnt here already - if there weren't enough bonds to burn, an error is thrown
        IERC20(collateralToken).safeTransfer(
            _msgSender(),
            collateralToSend
        );
        totalCollateral[collateralToken] -= collateralToSend;
        emit Converted(
            _msgSender(),
            collateralToken,
            amountOfBondsToConvert,
            collateralToSend
        );
    }
}

The corresponding tests add

const expectedCollateralToWithdraw =
    ConvertibleBondConfig.convertibilityRatios.map((ratio) =>
      tokensToConvert.mul(ratio).div(utils.parseUnits("1", 18))
    );

and a new function getEventArgumentsFromLoop which allows for looping over the emitted events in a loop

args.forEach(
  (
    {
      convertorAddress,
      collateralToken,
      amountOfBondsConverted,
      amountOfCollateralReceived,
    }: any,
    index: number
  ) => {
    expect(convertorAddress).to.equal(bondHolder.address);
    expect(collateralToken).to.equal(
      ConvertibleBondConfig.collateralTokens[index]
    );
    expect(amountOfBondsConverted).to.equal(tokensToConvert);
    expect(amountOfCollateralReceived).to.equal(
      expectedCollateralToWithdraw[index]
    );
  }
);

vs something like

 {
      convertorAddress,
      collateralToken,
      amountOfBondsConverted,
      amountOfCollateralReceived,
  } = getArgsFromEvent(...);
  expect(convertorAddress).to.equal(bondHolder.address);
  expect(collateralToken).to.equal(
    ConvertibleBondConfig.collateralTokens[index]
  );
  expect(amountOfBondsConverted).to.equal(tokensToConvert);
  expect(amountOfCollateralReceived).to.equal(
    expectedCollateralToWithdraw)

I think overall multiple collateral adds a good amount of complexity. I think that's because the contract isn't terribly complex. Single collateral would be easier to think about. I think I've got a good understanding of handling arrays in solidity + tests at the moment.

from v1-core.

@jordanalexandermeyer

from v1-core.