x/gamm: Consider deleting IsActive and related checks

[AlpinYukseloglu]

Background

Consider deleting IsActive and related checks, including getPoolForSwap(). These checks were introduced for a feature that no longer exists (namely, letting people add/remove liquidity from a pool while disallowing swaps in it). Currently, IsActive is hard-coded to return true, and every check it is used in never fails. In the spirit of not keeping code that doesn't do anything in our codebase, I think it's worth considering removing this check and its related functions.

osmosis/x/gamm/pool-models/balancer/balancer_pool.go

Lines 473 to 475 in 85529c7

	func (pa Pool) IsActive(ctx sdk.Context) bool {
	return true
	}

Suggested Design

• Remove IsActive and getPoolForSwap()
• Replace uses of getPoolForSwap() with GetPoolAndPoke()

Acceptance Criteria

• All existing tests pass
  

[p0mvn]

The suggested design makes sense. This should be removed to avoid confusing people and opening up potential vectors for errors with the unused code in one of the core modules

[p0mvn]

Following up on the discussion offline:

• Can we add a comment for the method spec explaining what the method does and why we are keeping it while it always returns true? (e.g. for emergency situations to have an ability to quickly change the logic about when a pool is active)
• Can we have a simple unit test where we expect IsActive to always return true for now? This is to ensure that even such a basic function is covered by tests so that if anyone accidentally flips the flag, the unit test would immediately catch it

[AlpinYukseloglu]

Following up on the discussion offline:

• Can we add a comment for the method spec explaining what the method does and why we are keeping it while it always returns true? (e.g. for emergency situations to have an ability to quickly change the logic about when a pool is active)
• Can we have a simple unit test where we expect IsActive to always return true for now? This is to ensure that even such a basic function is covered by tests so that if anyone accidentally flips the flag, the unit test would immediately catch it

Agreed for both. I think we also discussed adding a test that would cover the "flipping" functionality – @ValarDragon you briefly mentioned mock testing as an option. Do we have any examples of something like that in our codebase/how did you envision it would look?

[ValarDragon]

Mocks refer to making a sample implementation of an interface, satisfying the minimal testing needs wanted at that spot.

This is a tool that can help generating such an interface stub, but it may be easier to just make a struct in place that implements the Pool interface.

I think its fine to leave the latter as a future issue, and for now just having a test for the IsActive functionality that roman noted sounds good to me.

[AlpinYukseloglu]

Mocks refer to making a sample implementation of an interface, satisfying the minimal testing needs wanted at that spot.

This is a tool that can help generating such an interface stub, but it may be easier to just make a struct in place that implements the Pool interface.

I think its fine to leave the latter as a future issue, and for now just having a test for the IsActive functionality that roman noted sounds good to me.

Sounds good!