arcadia-finance / arcadia-vaults Goto Github PK

    Following tests are still missing:

factory.sol

• testSuccess_isVault_Negative (not a vault)
• testRevert_setNewVaultInfo_VersionRootIsZero
• testRevert_setNewVaultInfo_LogicAddressIsZero
• liquidate (all tests, both happy as unhappy)

vault.sol

• testRevert_initialize_InvalidVersion
• testRevert_openTrustedMarginAccount_OpeningAccountReverted
• testSucces_openTrustedMarginAccount
• testRevert_closeTrustedMarginAccount_NotSet
• testRevert_closeTrustedMarginAccount_NonZeroOpenPosition
• testSuccess_closeTrustedMarginAccount_CloseSetTrustedMarginAccount
• increaseMarginPosition (all tests, both happy as unhappy)
• decreaseMarginPosition (all tests, both happy as unhappy)
• getCollateralValue (all tests, both happy as unhappy)
• testRevert_liquidateVault_VaultIsHealthy
• testRevert_liquidateVault_FailedToStartAuction

I propose that we create separate ticket and already merge these

Originally posted by @Thomas-Smets in #146 (comment)

Currently waiting for an update in the forge std library that allows us to change specific data in a structure w cheat codes

PR is in and will probably merge within next few weeks.

Shouldn't the vault/factory have its metadata updated after being liquidated?

Originally posted by @j-vp in #39 (comment)

Next to initial Issue, the metadata (tokenURI) should be worked out as separate feature.

https://github.com/arcadia-finance/lending-core/blob/108750a32a3424c86fbdaffa3ed8cf46759a595a/src/OracleHub.sol#L52-L54

Is it possible to do without baseAssetIsNumeraire by encoding it in baseAssetNumeraire? Eg. if baseAssetIsNumeraire is false, we can represent it in baseAssetNumeraire using 0. Then USD will be 1 in baseAssetNumeraire and so on for other numeraires.

To avoid the ifs in Vault.deposit(), the asset types could be returned as a list. to be gas profiled.

test for the unhappy flows of the liquidator

• LQ_BV: Debt repaid
• "LQ_BV: Not for sale"
• double claims
• claims not available yet
• claims when reserve fund is empty

different paths with and without surplus

Originally posted by @Thomas-Smets in #26 (comment)

    todo in a new issue: expand it for vaults with open positions

Originally posted by @j-vp in #155 (comment)

Implementation is good for now but when we optimize for gas usage, we might want to avoid the extra external contract call.
We now do a .getBalance() in the vault, to get the shares. And here we jump a second time to the ERC4626 contract to convert the shares to assets.
It's a trade-off between gas usage and keeping the vault generic (don't make a difference between logic to handle ERC20 and ERC4626).

https://github.com/arcadia-finance/lending-core/blob/bed0c91389a6ffea81749d93c3f2c923a9360ab0/src/Factory.sol#L194

On each vault creation, a mapping (isVault => bool) is populated.

If we start with vault ID 1 instead of 0, we can replace the isVault mapping with a vaultIndex[vaultAddress] > 0 to save 22187 gas on vault deployment.

          can these be packed?

Originally posted by @j-vp in #178 (comment)

https://github.com/arcadia-finance/lending-core/blob/4d3ac08b784f54300d3e1b9ba3757bba2148b20d/src/Factory.sol#L28

This variable used only for setting true

https://github.com/arcadia-finance/lending-core/blob/4d3ac08b784f54300d3e1b9ba3757bba2148b20d/src/Factory.sol#L334

This line will always be true then

Is this intended? we should set to false when? If we do not set to false, is variable needed?

https://github.com/arcadia-finance/lending-core/blob/6eb1f213245765cd7d7a0f8b562fecd8669a2c9e/src/Vault.sol#L40-L61

Why are some variables named with leading underscores (eg. Vault.sol, line 40 - 53) while others are not (Vault.sol, line 57 - 61)?

struct AssetInformation {
    uint256 idRangeStart;
    uint256 idRangeEnd;
    address assetAddress;
    address[] oracleAddresses;
  }

  mapping (address => AssetInformation) public assetToInformation;

in a mapping (assetAddress => ..), the return shouldn't contain the assetAddress anymore

cc @Thomas-Smets

    Name unclear to me

    struct AssetsAndRiskParameters {

Originally posted by @Thomas-Smets in #151 (comment)

File: Vault.sol

Perhaps we can add getLengths() to a mock vault contract? So this function won't have to be deployed to mainnet.

https://github.com/arcadia-finance/lending-core/blob/6eb1f213245765cd7d7a0f8b562fecd8669a2c9e/src/Vault.sol#L1030-L1048

    Why not:

require(inMainRegistry[assetAddress], "MR_BPD: Asset not in mainreg");
And remove if and else.
Also do we need the check, we don't do it in batchProcessWithdrawal?

Originally posted by @Thomas-Smets in #166 (comment)

    Might be cleaner to input variables separately and not in struct, and just fetch the parameters such as underlyingAssetUnit instead of checking it is equal.

Might be a different task.

Originally posted by @Thomas-Smets in #151 (comment)

If this fails -> reserveFund is empty, start procedure to sell equity tokens? Or what should happen?

Originally posted by @Thomas-Smets in #26 (comment)

https://github.com/arcadia-finance/lending-core/blob/6eb1f213245765cd7d7a0f8b562fecd8669a2c9e/src/Vault.sol#L271-L289

In line 273, it says note the safeTransferFrom() but the code calls transferFrom() in line 285. Which one should be used?

          Not in scope of this PR, but maybe a new ticket, should this be onlyOwner or should we work with permissions (-> keeper networks can trigger certain actions)

Originally posted by @Thomas-Smets in #176 (comment)