Allow overriding onSwap in Stable Pool

[EndymionJkb]

Description

Inspired by the LBP example, in which a custom pool overrides the onSwap hook, it strikes me that people might similarly want to make custom Stable pools, and would run into the same problem if it were not virtual.

In both cases, I think we should not make the invariant functions virtual, as they directly call Weighted/Stable math. If a custom pool uses different math, then it's not really a Weighted/Stable pool, and shouldn't be derived from it anyway.

Type of change

• Bug fix
• New feature
• Breaking change
• Dependency changes
• Code refactor / cleanup
• Optimization: [ ] gas / [ ] bytecode
• Documentation or wording changes
• Other

Checklist:

• The diff is legible and has no extraneous changes
• Complex code has been commented, including external interfaces
• Tests have 100% code coverage
• The base branch is either main, or there's a description of how to merge

Issue Resolution

[jubeira]

Interesting. But isn't onSwap just an optimized path defined by the invariant anyways? In other words, if you change the math there you're actually changing the invariant as well.

For LBPs I see that we're using it to block swaps. Why not just pause the pools instead? (see https://github.com/balancer/balancer-v3-monorepo/pull/1001/files#r1784418053)

[EndymionJkb]

Interesting. But isn't onSwap just an optimized path defined by the invariant anyways? In other words, if you change the math there you're actually changing the invariant as well.

It could be used to update an oracle or some other state in the pool too, even without the math changing. That could be done in a before hook, but that's more heavyweight (as it would cause everything to be reloaded unnecessarily, if the hook doesn't do anything that changes balances or rates). As with the LBP, doing stuff directly in the swap is more efficient.

For LBPs I see that we're using it to block swaps. Why not just pause the pools instead? (see https://github.com/balancer/balancer-v3-monorepo/pull/1001/files#r1784418053)

Answered in that PR; it's how LBPs usually work - you want to be able to start/stop the sale without affecting the owner's ability to add/remove liquidity. In production, they were often deployed first (even by us, in the beginning), and funded later from a different account, and you don't want people trading it in a low-liquidity state. The owner could set a pause manager, then wrap liquidity operations with unpause/pause, but doing it independently is simpler.

[jubeira]

It could be used to update an oracle or some other state in the pool too, even without the math changing. That could be done in a before hook, but that's more heavyweight (as it would cause everything to be reloaded unnecessarily, if the hook doesn't do anything that changes balances or rates). As with the LBP, doing stuff directly in the swap is more efficient.

But onSwap is actually view, so it still can't be used to update any sort of state.

Answered in that PR; it's how LBPs usually work - you want to be able to start/stop the sale without affecting the owner's ability to add/remove liquidity. In production, they were often deployed first (even by us, in the beginning), and funded later from a different account, and you don't want people trading it in a low-liquidity state. The owner could set a pause manager, then wrap liquidity operations with unpause/pause, but doing it independently is simpler.

Ok, that's fair as long as unbalanced liquidity operations are disabled.

[EndymionJkb]

It could be used to update an oracle or some other state in the pool too, even without the math changing. That could be done in a before hook, but that's more heavyweight (as it would cause everything to be reloaded unnecessarily, if the hook doesn't do anything that changes balances or rates). As with the LBP, doing stuff directly in the swap is more efficient.

But onSwap is actually view, so it still can't be used to update any sort of state.

It doesn't have to be view, though; it's not view in IBasePool, so we could make it non-view in both Weighted and Stable pools. It would still be view "in practice" for our pools, but we could declare them plain 'virtual' so that people could override and change local state, if we want to enable that in derived pools.

Answered in that PR; it's how LBPs usually work - you want to be able to start/stop the sale without affecting the owner's ability to add/remove liquidity. In production, they were often deployed first (even by us, in the beginning), and funded later from a different account, and you don't want people trading it in a low-liquidity state. The owner could set a pause manager, then wrap liquidity operations with unpause/pause, but doing it independently is simpler.

Ok, that's fair as long as unbalanced liquidity operations are disabled.

Since adding liquidity is permissioned, we let them do whatever they want. In rare cases, they can adjust the price by adding unbalanced (e.g., if demand has driven it up too high, they can add more project tokens, or they can restart the sale at a different price when it goes too low, etc.)

[EndymionJkb]

Should we make this (and WeightedPool) non-view here to enable local state-changing operations in derived pools?