Rename Record to PoolAsset #75
Conversation
ValarDragon
changed the title
|
The last two commits fixes a small amount of typos, error messages & panic messages. I think they're pretty minor, so not separating them into a standalone PR. |
| SetPoolAsset(denom string, PoolAsset PoolAsset) error | ||
| GetPoolAssets(denoms ...string) ([]PoolAsset, error) | ||
| SetPoolAssets(PoolAsset []PoolAsset) error | ||
| GetAllPoolAssets() []PoolAsset |
There was a problem hiding this comment.
I do wonder if we should rename these methods to remove the Pool prefix.
There was a problem hiding this comment.
Technically PoolAsset includes the weight, and is a bit different from the typical notion of 'assets' so do we really need to change it? But then again, I'm going to assume whoever is looking at the code would know it. So I'm fine either way.
There was a problem hiding this comment.
Ok, sounds good. I do think theres probably a better name than PoolAsset as well, since it also includes the weight, but I think this name is significantly more descriptive
ValarDragon
changed the title
|
Should we maybe call this "WeightedAmmAsset"? Eventually, the assets in a pool will be different than the assets in the amm (new pool types might keep some things in reserve, but not in the amm itself. For example, the Balancer v2 asset managers) |
But this logic is explicitly only for Balancer v1 pools. Perhaps |
closes #70
We should in a follow-on PR start working on documenting this module generally.
Question, should this be PoolReserves instead of PoolAsset?