Feature: Make upgradeCapitalPool copy over investmentAssets and coverAssets when pool is upgraded

[0xdewy]

Context

closes #473

Changes proposed in this pull request

This PR changes the capital pool so when it is deployed, it copies over coverAssets, investmentAssets and swapDetails from the previous pool. It also also adds the option to add additional assets when deploying a new Pool.

Test plan

A test was added to unit/Pool/upgradeCapitalPool.js. I was going to create a new unit test file constructor.js and add more tests, but the tests are still using truffle/web3 and I was having issues testing the reverts in the constructor.

Checklist

• Rebased the base branch
• Attached corresponding Github issue
• Prefixed the name with the type of change (i.e. feat, chore, test)
• Performed a self-review of my own code
• Followed the style guidelines of this project
• Made corresponding changes to the documentation
• Didn't generate new warnings
• Didn't generate failures on existing tests
• Added tests that prove my fix is effective or that my feature works

Review

When reviewing a PR, please indicate intention in comments using the following emojis:

• 🍰 = Nice to have but not essential.
• 💡 = Suggestion or a comment based on personal opinion
• 🔨 = I believe this should be changed.
• 🤔 = I don’t understand something, do you mind giving me more context?
• 🚀 = Feedback

[danoctavian]

At a feature spec level, this implementation makes things a little bit less flexible.

With the constructor variables we can rebuild the arrays everytime and add new things in on a need basis or deprecate etc.

Here we need to upgrade the pool and the run another governance update to add or deprecate an asset.

I'm not convinced it's ideal to have it, it does heavily reduce the probability of error so that's the big upside.

Putting this out there.

[0xdewy]

The lastSwapTime is no longer manually set in the constructor, but instead goes through _addAsset(), which sets lastSwapTIme to 0. I didn't make any changes for this as I wanted to ask if this exact timestamp is still required?

[0xdewy]

Should we exclude abandoned assets from being copied over? I could change this function so it only returns active assets.

[danoctavian]

what do you mean by abandoned what do you check for that?

[danoctavian]

there's no substraction here the comment is not relevant imo, it's just ETH skipped at the start

[danoctavian]

we can use some unit tests for the constructor itself for Pool.sol now since it's relatively complex.

[danoctavian]

Small changes, and a request for unit testing the constructor

[shark0der]

As per discussion, the current implementation will fail to upgrade because the previousPool.getAssetsAndSwapDetails() call will fail. Will close this task and we'll get back to it post V2 launch.