yearn / yearn-vaults-v2-subgraph

26 stars 41 forks source link

Add DelegatedAssets support for strategies #186

Closed Majorfi closed 1 year ago

Majorfi commented 2 years ago

Some strategies have a delegatedAssets field, used to indicate how many wantToken are deposited, from a strategy, to another Yearn Vault.
We should track this value in order to avoid counting twice theses token when computing the TVL.

Exemple contract here: https://etherscan.io/address/0xF9fDc2B5F60355A237deb8BD62CC117b1C907f7b#readContract

rareweasel commented 2 years ago

Hey @Majorfi thanks for creating this feature.

So the idea is to create a new delegatedAssets field. I wonder when we should update this value, every time a user deposits/withdraws funds from a vault?

Majorfi commented 2 years ago

Hum, my take would be to update that on harvest(), as it's not a field on the vault but on the strategy. I will ask around!

rareweasel commented 2 years ago

Hum, my take would be to update that on harvest(), as it's not a field on the vault but on the strategy. I will ask around!

yes you're right. Totally makes sense

wavey0x commented 2 years ago

this value can be affected anytime the strategy completes an investment or withdraw. i.e. harvests, withdraws, tends

the purpose is not meant for tvl double-counting (though it can be used this way), but rather as the native mechanism in the vault to prevent double-counting (or even triple-counting) of management fees when one user's deposit travels through 2 vaults.

0xkofee commented 2 years ago

Hmmm like @wavey0x was saying. the delegatedAssets is simply:

function delegatedAssets() public override view returns (uint256) {
        return vault.strategies(address(this)).totalDebt;
    }

and the totalDebt on the strategy can be changed through: withdraw -> reportLoss harvest -> report

so should I implement changing the delegatedAssets on both withdraw and harvest? Implementing for withdraw as well seems like a lot... might make the subgraph slower. What do you think @Majorfi @rareweasel

PR for just updating value on harvest: https://github.com/yearn/yearn-vaults-v2-subgraph/pull/187

rareweasel commented 2 years ago

Hmmm like @wavey0x was saying. the delegatedAssets is simply:

function delegatedAssets() public override view returns (uint256) {
        return vault.strategies(address(this)).totalDebt;
    }

and the totalDebt on the strategy can be changed through: withdraw -> reportLoss harvest -> report

so should I implement changing the delegatedAssets on both withdraw and harvest? Implementing for withdraw as well seems like a lot... might make the subgraph slower. What do you think @Majorfi @rareweasel

PR for just updating value on harvest: #187

As Wavey mentioned, we should implement in both cases. The subgraph is very slow for multiple reasons. I would say, let's implement both cases, trying not making rpc call in the subgraph. Wdyt @0xkofee @wavey0x ?