balancer / balancer-v3-monorepo

GNU General Public License v3.0
38 stars 15 forks source link

Clear _currentSwapTokensOut and _currentSwapTokensIn every each operations in BatchRouter #736

Closed elshan-eth closed 2 months ago

elshan-eth commented 3 months ago

Description

This PR fixed a problem with _currentSwapTokensIn/_currentSwapTokensOut token sets in BatchRouter. We cleared only _currentSwapTokenInAmounts, _currentSwapTokenOutAmounts, and _settledTokenAmounts at the end of the operation but didn't do it for _currentSwapTokensOut/_currentSwapTokensIn. This problem happens every time we have multiple operations in one transaction.

In the first iteration, I tried to create an index for all mappings which we iterate each operation. It means that if the index changes, we don't need to clear every property. This way had a problem with stack too deep (without viaIR compilation) and gas was increased.

This way is simple and cheap.

NOTE: merge this PR after #731

Type of change

Checklist:

Issue Resolution

Closes #707

elshan-eth commented 2 months ago

The code seems nice, but just a question: If amounts is cleared and the amount of token to transfer is 0, why do we need to clear tokensIn and tokensOut, if effectively that token won't be transferred? I mean, looks like we added at least 1k gas to do this. Isn't it better to check if amount is greater than 0 before transfering the asset from/to the user during settle?

if there will be several calls within 1 transaction (I don’t know if this makes sense), it would be the same. I mean we can remove elements and it will cost around 100 gas but also if we don't remove this the next call will read it to compare it with zero and also pay 100 gas more.