Refactor ManagedPool constructor arguments #2078
Conversation
|
I have an alternative in the works as part of the solution for #2026 that also reduces the stack pressure, but it's a bit more extensive to all pools (not just managed). I did a 'similar' argument split in the settings in #2075 when trying to add the version, but the result is pretty ugly: adding the version there doesn't feel right (with this split here it could work though). While I think the split in this PR makes sense overall, I think we'll need some more work when we add the version to every pool. Can we hold this one for a bit and compare to the broader refactor? |
|
@nventuro can we please evaluate this and #2080 in parallel? I think #2080 should also solve the stack pressure and give you more room, while also solving the version for good. Again, we can also proceed with this and deal with the version later, but at some point when we move the version to the base layer we won't be able to dodge a larger refactor (weighted and stable are also tight with the stack). |
There was a problem hiding this comment.
LGTM! Just one comment regarding JS types.
At a conceptual level I find it a bit weird to group name and symbol in a struct that applies just to managed, but for the purposes of this PR it should be fine. I do like the concept of factory vs user provided params 👍
| @@ -204,11 +204,8 @@ export type ManagedPoolRights = { | |||
| }; | |||
|
|
|||
| export type ManagedPoolParams = { | |||
There was a problem hiding this comment.
I would consider refactoring these types so that they match the ones in the contracts.
This one e.g. is the settings params, and we are missing the new ManagedPoolParams and ManagedPoolConfigParams.
There was a problem hiding this comment.
I didn't do the refactor because I'm not a fan of how this ended up working (as mentioned in the original comment), and would rather change how this whole thing works tbh.
|
We don't actually need to worry about factory vs user provided, since we don't have to expose that same struct on the factory. We could (should) have entirely different structs for In this case it was convenient however, since we don't have said decoupling ready. But it doesn't have to remain this way. |
There was a problem hiding this comment.
I especially like isolating the factory (Config) params.
We might have burned the branch name prematurely though; I'm sure there will be worse things later. It does seem like every time we breathe on this code we need to struggle for hours fitting it into the stack, so any help with that is very welcome. I call dibs on '-and-the-horse-it-rode-in-on'
| address owner, | ||
| uint256 pauseWindowDuration, | ||
| uint256 bufferPeriodDuration | ||
| address[] memory asssetManagers, |
There was a problem hiding this comment.
| address[] memory asssetManagers, | |
| address[] memory assetManagers, |
| params.tokens, | ||
| params.assetManagers | ||
| settingsParams.tokens, | ||
| asssetManagers |
There was a problem hiding this comment.
| asssetManagers | |
| assetManagers |
| @@ -54,7 +55,6 @@ export type WeightedPoolDeployment = { | |||
| owner?: string; | |||
| admin?: SignerWithAddress; | |||
| from?: SignerWithAddress; | |||
| mockContractName?: string; | |||
There was a problem hiding this comment.
This should also be removed from TypesConverter.ts.
Description
This modifies the constructor of ManagedPool and ManagedPoolSettings so that there's more structs and fewer variables, reducing stack pressure and allowing us to add more constructor parameters.
We now have three structs:
This means the factory can create its params while the user provides the other two structs. The owner is standalone since the factory will likely create the owner, so we don't want to put it in the structs.
We could alternatively have the factory have its own structs, decoupled from the Pool's (which makes a lot of sense), but that was a bigger change.
This was much more painful than it should've been, and debugging it was extremely annoying, having to chase params and deployments in multiple places. It shows our helpers are in dire need of modernization - WeightedPoolDeployer is an unholy abomination that is capable of deploying a bajillion unrelated things in multiple incompatible ways, and needs to head to the chopping block.
Type of change
Checklist:
master, or there's a description of how to mergeIssue Resolution
Part of #2042