[New Operator] - Beefy Single Asset Vault #107

maximebrugel · 2022-03-06T09:14:55Z

New operator to deposit/withdraw in Beefy => https://app.beefy.finance/#/bsc
On operator deployed is for one vault, and can be deployed on every chains where Beefy is available.

How the operator works

Deposit:
=> Use vault underlying asset (input).
=> Deposit this token in Beefy.
=> Receive the "moo" token, the share of the vault (output).
Withdraw:
=> Use moo token (input).
=> Deposit the moo token by calling withdraw (will burn the moo token).
=> Receive the underlying asset (output).

Test changes

Now, we can specify the describe that we want for our tests : describeOnBscFork or describeWithoutFork. Because we don't want to run all tests on a BSC fork.

There are also new .env variables :

FORKING="false"
FORK_CHAINID=""
FORK_URL=""

Note: I've been using Chainstack to run a full BSC node (the request limit is very good). The standard RPC endpoint was not working well for forking.

adrien-supizet

Well done 👏 Let's discuss the relationship vault <-> operator further.
It's on the right track 🎉

adrien-supizet · 2022-03-07T12:02:12Z

contracts/operators/Beefy/BeefyVaultOperator.sol

+import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+
+/// @title Beefy Single Vault Operator
+/// @notice Deposit/Withdraw in a Beefy vault (native or non-native).


It might require a bit of work, but I suggest we look into supporting multiple vaults per operator.
1<->1 relationships between operators and vaults seem pretty cumbersome to maintain and operate.

Let's discuss pros and cons

The main argument is to avoid the operator storage, with token and vault immutable (save gas), and also having the same process for adding/removing operations.
But it's easier to handle one operator with multiple vaults. So I think that both are fine.

While operating the protocol, it is probably easier to manage one operator + one storage contract, rather than 10s of operators. This is true for the admin deploying contract but it also makes it easier to plug new vaults to the frontend, and route the users between the vaults

I understand it will be slightly more costly for users though. But relatively to what Beefy requires as gas, I'm not sure it would make a significant difference.

ok let's do this!

adrien-supizet · 2022-03-07T11:57:06Z

.env.example

+FORKING="false"
+FORK_CHAINID=""
+FORK_URL=""


Those env variables don't seem secret to me. Consider adding them to the example file to make life easier for other devs.

adrien-supizet · 2022-03-07T12:00:25Z

test/unit/ExchangeHelpers.unit.ts


-describe("ExchangeHelpers", () => {
+describeWithoutFork("ExchangeHelpers", () => {


💅 I think you could have simply created the new describeWithFork and used the regular describe otherwise, so that you didn't have to change all existing describe clauses.

No because describeWithoutFork is used to exclude forking tests. Using describe means that both tests are executed.

For example, I don't want to execute NestedFactory tests and the DummyRouter with a fork. It's useless, I want to skip these tests while forking

I mean, default is both fork and no fork and you specify for no fork, I was suggesting the other way around. But it does not matter much 👍

adrien-supizet · 2022-03-07T12:06:29Z

contracts/operators/Beefy/BeefyVaultOperator.sol

+    ///         - [1] : The token deposited address
+    function deposit(uint256 amount, uint256 minVaultAmount)
+        external
+        payable


❔ Should this function be payable? The asset handled seem to always be of ERC20 type.

create and processInputOrders are payable. Because deposit is called via delegatecall it needs to be payable :)

adrien-supizet · 2022-03-07T12:11:29Z

contracts/operators/Beefy/BeefyVaultOperator.sol

+        uint256 vaultBalanceBefore = vault.balanceOf(address(this));
+        uint256 tokenBalanceBefore = token.balanceOf(address(this));
+
+        ExchangeHelpers.setMaxAllowance(token, address(vault));


From what I understand, all tokens in this contract will be in "transit" and never stored permanently.
I'm starting to think that we should remove setMaxAllowance uses. But here it would be fine anyway if they're not stored.

Beefy needs to transferFrom the underlying token from NestedFactory, so we need to approve the vault.

Yes obviously, my comment is about maxAllowance vs increaseAllowance

oh okay!
I think that maxAllowance is better because it's handling the tokens race condition.

Is it?

To me, it's great for gas opt, but I don't think race conditions are a consideration here.
Systematically, allowance is increased by an amount before the amount is transferred, and there are no concurrent transactions as transactions are isolated and executed in order

If there can be race conditions here I'm curious to know more about it

I mean, we are not trying to handle race conditions, but tokens that are handling race conditions. So, only using "increaseAllowance" will fail with some tokens.

adrien-supizet · 2022-03-07T12:20:36Z

contracts/operators/Beefy/BeefyVaultOperator.sol

+    /// @notice Deposit the asset in the Beefy vault and receive
+    ///         the vault token (moo).
+    /// @param amount The token amount to deposit
+    /// @param minVaultAmount The minimum vault token amount expected


❔ I need more context as to why this variable is useful. I believe it acts as an equivalent to slippage control, and ensures good behavior. I'll look more in-depth in the docs to see how the vault tokens are emitted and why this should be passed as a parameter.
Please mention any other reason this variable is useful in the meantime.

As a matter of principle, I think that is better to specify the minimum amount that we want to receive.
It's not critical with a vault, but it's good to guarantee that a user will have this amount of "share".

Where does the number come from? Passed from the frontend?

And I guess this is read from a method of the vault contract

Yes, we need to "estimate" the amount before (backend). But it can be zero.

adrien-supizet · 2022-03-07T13:05:26Z

test/unit/BeefyVaultOperator.unit.ts

+            const tx = await context.nestedFactory
+                .connect(context.user1)
+                .create(0, [{ inputToken: ETH, amount: bnbToDepositAndFees, orders, fromReserve: false }], {
+                    value: bnbToDepositAndFees,
+                });


🔧 I would like to also see a test case for deposits post-creation using the new process orders method

maximebrugel · 2022-03-10T19:55:40Z

@adrien-supizet Changes are done ✅

adrien-supizet

Looks good to me 👏
Minor comments to look at.

adrien-supizet · 2022-03-14T17:15:34Z

contracts/operators/Beefy/BeefyVaultOperator.sol

-        returns (uint256[] memory amounts, address[] memory tokens)
-    {
+    function deposit(
+        IERC20 vault,


💅 vault should be of type address here:

it matches the natspec description

it is easier to understand

you don't have to cast it to address L42

For the sake of consistency, it is passed as addresses in the constructor already

adrien-supizet · 2022-03-14T17:16:57Z

contracts/operators/Beefy/BeefyVaultOperator.sol

    /// @param amount The vault token amount to withdraw
    /// @return amounts Array of amounts :
    ///         - [0] : The token received amount
    ///         - [1] : The vault token deposited amount
    /// @return tokens Array of token addresses
    ///         - [0] : The token received address
    ///         - [1] : The vault token deposited address
-    function withdraw(uint256 amount) external returns (uint256[] memory amounts, address[] memory tokens) {
+    function withdraw(IERC20 vault, uint256 amount)


💅 Same comment about vault type

adrien-supizet · 2022-03-14T17:24:04Z

contracts/operators/Beefy/BeefyVaultStorage.sol

+    function addVault(address vault, address token) external onlyOwner {
+        require(vault != address(0), "BVS: INVALID_VAULT_ADDRESS");
+        require(token != address(0), "BVS: INVALID_TOKEN_ADDRESS");
+        vaults[vault] = token;


❔ This method allows to override the current token address for a given vault.
We could add a require to check that the vault does not exist yet, enforcing the owner to delete a vault and re-add it if the token address changes.
In most cases, it can prevent overriding by mistake.

fix: remove cast from parameter (beefy operator)

maximebrugel added the To review Let people know this PR is ready for a review label Mar 6, 2022

maximebrugel requested review from oguimbal and adrien-supizet March 6, 2022 09:14

adrien-supizet reviewed Mar 7, 2022

View reviewed changes

adrien-supizet removed the To review Let people know this PR is ready for a review label Mar 8, 2022

adrien-supizet approved these changes Mar 14, 2022

View reviewed changes

adrien-supizet added the Fix and merge Minor changes required label Mar 14, 2022

maximebrugel added 9 commits March 14, 2022 22:46

feat: Beefy and ZeroExBeefy Operators

48036e9

test: skip option for forking tests

4e96964

build: use forking "enabled"

322ad55

test: Beefy vault deposit test

2d96c5b

test: beefy withdraw test

d31525e

remove ZeroExBeefy operator + new tests

efcaa48

test: remove swaptarget test for beefy

15afb78

feat: List vaults in beefy operator

7e27f2e

test: add/remove vault tests

f68d6d8

maximebrugel force-pushed the beefy-operator branch from 07f3ced to f68d6d8 Compare March 14, 2022 21:47

fix: require already existent vault

98e1cdf

fix: remove cast from parameter (beefy operator)

maximebrugel merged commit f9ec12e into master Mar 14, 2022

maximebrugel deleted the beefy-operator branch March 14, 2022 22:06

maximebrugel mentioned this pull request May 4, 2022

[New Operator] - Beefy LP Vault #114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New Operator] - Beefy Single Asset Vault #107

[New Operator] - Beefy Single Asset Vault #107

maximebrugel commented Mar 6, 2022

adrien-supizet left a comment

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022 •

edited

Loading

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

adrien-supizet Mar 7, 2022

maximebrugel Mar 7, 2022

maximebrugel commented Mar 10, 2022

adrien-supizet left a comment •

edited

Loading

adrien-supizet Mar 14, 2022 •

edited

Loading

adrien-supizet Mar 14, 2022

adrien-supizet Mar 14, 2022 •

edited

Loading


		describe("ExchangeHelpers", () => {
		describeWithoutFork("ExchangeHelpers", () => {

[New Operator] - Beefy Single Asset Vault #107

[New Operator] - Beefy Single Asset Vault #107

Conversation

maximebrugel commented Mar 6, 2022

How the operator works

Test changes

adrien-supizet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrien-supizet Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximebrugel commented Mar 10, 2022

adrien-supizet left a comment • edited Loading

Choose a reason for hiding this comment

adrien-supizet Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrien-supizet Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

adrien-supizet Mar 7, 2022 •

edited

Loading

adrien-supizet left a comment •

edited

Loading

adrien-supizet Mar 14, 2022 •

edited

Loading

adrien-supizet Mar 14, 2022 •

edited

Loading