Move Compound approve to mapTokenToCToken

[GeraldHost]

Is it possible to move the token approvals for the cToken into the mapTokenToCToken function?

Can we remove the approvals in this function and instead to a max approval in mapTokenToCoToken similarly to how we do it in the Aave adapter function mapTokenToAToken?

function deposit(address tokenAddress) external override checkTokenSupported(tokenAddress) {
    // get cToken
    IERC20Upgradeable token = IERC20Upgradeable(tokenAddress);
    address cTokenAddress = tokenToCToken[tokenAddress];
    CToken cToken = CToken(cTokenAddress);
    uint256 amount = token.balanceOf(address(this));
    // mint cTokens
-    token.safeApprove(cTokenAddress, 0);
-    token.safeApprove(cTokenAddress, amount);
    uint256 result = cToken.mint(amount);
    require(result == 0, "Error minting the cToken");
}  

I checked the cToken mint code and I can't see that it resets the approval ever so feel like this should be possible. I believe this would save gas when users stake.

[maxweng]

The reason we had to reset the allowance is that for some ERC20 tokens, like USDT, its allowance needs to be 0 before approving a new amount. So we want to make sure the adapter is future-prove.

[GeraldHost]

@maxweng should we not account for this in the other adapters if that is the case? Might be worth seeing how much gas a change like this saves, considering @kingjacob matra I can't imagine we want to support USDT ever 😂 Stake is super expensive right now so anything we can save is good to consider.

[maxweng]

yea, let's run the gas delta tests to see the difference between those scenarios.

[maxweng]

actually, I'm thinking if we really want to save gas for staking, we can make batch deposits to aave/compound, instead of doing it for each stake. it's the minting action that takes A LOT of gas.

[GeraldHost]

@maxweng I like that idea! I was also wondering if we could remove comptroller.withdrawRewards(msg.sender, stakingToken); Or at least stick it behind a boolean so that if you are staking for the first time you don't have to also claim rewards as this claim is expensive. They did something similar in MasterChef V2 for this reason.

[maxweng]

I guess we could have a check to see if it's a new user or if there're remaining rewards before calling the claim function. but that's only gonna save gas once for each new user but will add a bit extra cost to all others. so we may have to weigh our options here.

[maxweng]

and the reason batch deposit's not there yet is that we haven't found a good way to incentivize 3rd party to actually trigger the deposit for others. do you have any ideas for that?

[GeraldHost]

@maxweng For the claim I mean we should just have a second arguments bool claim which we can toggle in the UI. Because at the moment the UI has support for a zero amount stake which is basically just a claim and most users will stake and then use that. I think it would drop the stake gas quite a bit and get more people staking. Some people didn't stake because the cost is high with this lookup.

[maxweng]

Gotcha. we can add a claim toggle in that case. but I don't see that will have a big impact on the gas costs because the heavy lifting part is to calculate how much rewards the user accrues up till now, no matter if he claims right away or not. so the only gas cost could be reduced is that of the token transfer part.