Handling ERC4626 vaults in AcreRouter #62
Conversation
AcreRouter contract should manage ERC4626 vaults within the Acre system. Owner of the contract should be able to add or remove vaults.
Upgradability will be handled down the road in a separate PR(s)
core/test/AcreRouter.test.ts
Outdated
| await acreRouter.connect(governance).addVault(vault3) | ||
|
|
||
| expect(await acreRouter.vaults(0)).to.equal(vault1) | ||
| const isVault1Approved = await acreRouter.vaultsInfo(vault1) |
There was a problem hiding this comment.
That's interesting - it returns boolean when the vaultsInfo is a map of Vaultstruct. I would expect the struct even if there is one field in the structure.
There was a problem hiding this comment.
Yea, looks like Solidity compiler optimizes the use of Struct with 1 var and we can reference our boolean as if it was just a boolean with no Struct.
- Renamed VauldAdded->VaultAuthorized and VaultRemoved->VaultDeauthorized events - Returning vaults array instead of vaults array length
- Replaced Address type with a String type - Added test fixture just like we have in Acre.test.ts - Various renames
| /// implemented externally. As long as it complies with ERC4626 | ||
| /// standard and is authorized by the owner it can be plugged into | ||
| /// Acre. | ||
| address[] public vaults; |
There was a problem hiding this comment.
Just wonder if we need this array. From the dapp/subgprah perspective, we can get the vault address from an event. Not sure about the contracts perspective, maybe other contracts will need this?
There was a problem hiding this comment.
This array will track all the vaults within Acre ecosystem. vaultsInfo map won't iterate through all the vaults in Acre, you need to know and provide a concrete address. From the dApp perspective maybe it's easy tracking all the authorize/deauthorize events and produce the final result what's in the system, but if a user just want to go to etherscan and see the vaults addresses that won't make it. Besides, it's way easier just to call vaults() and get all the vaults rather than fetching events and then combine them to get the final answer.
Depends on: #58 Improve test fixtures used in Acre contract tests: - use Acre and TBTC contracts deployed with hardhat development scripts (#58) - use unnamed signers as stakers, to not overlap with named signers defined in `hardhat.config.ts`. `ethers.getSigners()` returns all signers, regardless of whether it is a Hardhat named or unnamed account --- This PR contains changes from #62 which were unintentionally merged to this branch.
| error VaultAlreadyAuthorized(); | ||
| error VaultUnauthorized(); |
There was a problem hiding this comment.
We have a warning here. We should define errors below the events declaration.
There was a problem hiding this comment.
👍 I'll change that in the following PR
AcreRouter contract should manage ERC4626 vaults within the Acre ecosystem. Owner of the contract should be able to add or remove vaults.
There's a
Vaultstruct inside of theAcreRouterthat holds only one booleanapproved. This struct will grow in the next PR(s) adding things like "distribution percent" for each vault.