Pool state updated within policy function

[markusbkoch]

Congrats on the work, everyone - this looks very good and will be super useful to the TE and Balancer communities.

I'd like to suggest an improvement: IIUC, the combination of the object oriented nature of your architecture plus the current logic defined in p_action_decoder is causing the pools state to be updated within the policy function and not the state update function.

eg

BalancerPools_Model/model/parts/system_policies.py

Lines 103 to 107 in 9489bb0

	def p_join_pool(params, step, history, current_state, action):
	"""
	Join a pool by providing liquidity for all token_symbols.
	"""
	pool = current_state['pool']

...

BalancerPools_Model/model/parts/system_policies.py

Lines 126 to 127 in 9489bb0

	pool['tokens'][symbol].balance += amount
	pool['pool_shares'] = Decimal(pool['pool_shares']) + pool_amount_out

I verified this initial impression by commenting out this line and observing that the simulation returned the same results:

BalancerPools_Model/model/partial_state_update_block.py

Line 17 in 9489bb0

'pool': s_update_pool,

The implication is that all state update functions within the partial state update block read the updated pool state instead of the state at the previous substep as would be expected. Which doesn't break anything for this particular model because all other state variables are essentially metrics.

It might seem like a nitpick, but the separation of policy functions and state update functions and the purposes of each one is an issue we frequently face when introducing cadCAD concepts. cadCAD's flexibility enables us to do basically anything as long as we know what we're doing, but standardizing this sort of thing is IMO key to increasing the library's user base. More importantly, future versions might count on this best practice being the norm and introduce what would be breaking changes from this model's perspective.

My suggestion would be to split the current p_action_decoder into:

1. a policy function that reads from action_df and returns action metadata
2. a state update function that actually performs the updates

This would in turn require including a second partial state update block for computing metrics like the spot price

[randomshinichi]

Not a nitpick - I've always felt that cadCAD should be stricter about some things. What things exactly, while not restricting flexibility too much? I have no idea. I think a good place to start thinking would be some form of standardization to allow cadCAD models to plug into other cadCAD models @markusbkoch