5 - closePositions respects minimumTransactionAmount (depends on 4) #49
Conversation
mcclurejt
changed the title
contracts/EverlongStrategy.sol
Outdated
| // Calculate the amount of bonds needed to reach the minimum | ||
| // transaction amount. | ||
| uint256 minimumBonds = minimumTransactionAmount().mulDivUp( | ||
| position.bondAmount, | ||
| totalPositionValue | ||
| ); |
There was a problem hiding this comment.
when you're closing longs, the minimum tx amount refers to bond amounts, you don't need to do any base <> share conversions then. just use _poolConfig.minimumTransactionAmount.
if (_bondAmount < _minimumTransactionAmount) {
revert IHyperdrive.MinimumTransactionAmount();
}There was a problem hiding this comment.
So I don't think we even need to do this calculation to convert it to base (or share) units.
There was a problem hiding this comment.
Got it! Thanks for pointing this out. Included the fix
| // We need to close at least the `minimumTransactionAmount` | ||
| // value of bonds. It's safe to assume that the position | ||
| // has at least that much. | ||
| bondsNeeded = bondsNeeded.max(minimumBonds); |
There was a problem hiding this comment.
see above: shouldn't this just be _poolConfig.minimumTransactionAmount?
There was a problem hiding this comment.
After applying changes from above I decided to leave minimumBonds as a variable with a comment on why conversion isn't necessary.
I know it's a bit more gas but I feel like it's worth it for the readability/explanation.
Happy to change if you think otherwise :)
Currently, `closePositions(..)` does not check whether a partial closure will result in a position value less than the minimum transaction amount. This PR adds the necessary logic for this check as well as a test to ensure it is behaving as expected. [issue](https://cantina.xyz/code/4f25dfd5-d3e6-4e7a-9481-d7306b795f2b/findings/5)
mcclurejt
force-pushed
the
mcclurejt/fix/close-positions-respect-min-tx-amount
branch
from
e6060fd to
8464f8c
Compare
No conversions are needed for min tx amount when closing longs since it's denominated in bonds not execution tokens
Currently,
closePositions(..)does not check whether a partial closure will result in a position value less than the minimum transaction amount.This PR adds the necessary logic for this check as well as a test to ensure it is behaving as expected.
issue
depends on changes from the fix for 4