Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MetaMorpho D3M #117

Merged
merged 76 commits into from
Mar 30, 2024
Merged

MetaMorpho D3M #117

merged 76 commits into from
Mar 30, 2024

Conversation

hexonaut
Copy link
Contributor

@hexonaut hexonaut commented Mar 15, 2024

D3M into MetaMorpho Vault with a multisig operator.

@DaiFoundation-DevOps
Copy link

DaiFoundation-DevOps commented Mar 15, 2024

CLA assistant check
All committers have signed the CLA.

@hexonaut hexonaut changed the title Add operator plan and tests Morpho D3M Mar 15, 2024
Copy link
Contributor Author

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor issues. Looks good otherwise.

src/pools/D3M4626TypePool.sol Outdated Show resolved Hide resolved
src/pools/D3M4626TypePool.sol Outdated Show resolved Hide resolved
Copy link
Contributor Author

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok both contracts LGTM. Will do test review shortly.

@sunbreak1211
Copy link
Contributor

@hexonaut let's add the date to the audit file name.

@MathisGD

This comment was marked as outdated.

test: hardcode block before compound 227
@SidestreamColdMelon
Copy link

While writing integration test for this module in the 2024-03-26 spell, I noticed that for the previous D3Ms we've tested disabling the plan with via MOM and effectively bringing art and ink to ~0, see (SPARK, AAVEV2, COMPV2). Doing the same with this module currently doesn't seem to work. Am I missing something obvious?

@hexonaut
Copy link
Contributor Author

While writing integration test for this module in the 2024-03-26 spell, I noticed that for the previous D3Ms we've tested disabling the plan with via MOM and effectively bringing art and ink to ~0, see (SPARK, AAVEV2, COMPV2). Doing the same with this module currently doesn't seem to work. Am I missing something obvious?

Sorry forgot to mention about this. I added a response to fix: makerdao/spells-mainnet#402 (review)

@SidestreamColdMelon
Copy link

SidestreamColdMelon commented Mar 26, 2024

FYI: D3MInit.sol imports DssInstance from the dss-test and requires it to be passed to the init scripts. This implies importing MCD library in the spell, which in turn imports GodMode and vm contracts, which eventually leads to them being deployed with the spell. This at least significantly increases deployment transaction cost. Note that this pattern is present across past spells as well, see spark D3M being deployed in 2023-04-28. (But as current spell is already deployed, it should be irrelevant for this PR, therefore explicitly leaving it as FYI)

@sunbreak1211
Copy link
Contributor

@hexonaut two comments:

  • very minor: can we change the audit file name to: 2024_03_Spearbit-report-maker-d3m-implementation-for-morpho-review. It seems easier for who is looking at the list to identify the file.
  • who and when will be taking care to remove the dependencies so we can finish the PR and merge it?

@hexonaut
Copy link
Contributor Author

@hexonaut two comments:

  • very minor: can we change the audit file name to: 2024_03_Spearbit-report-maker-d3m-implementation-for-morpho-review. It seems easier for who is looking at the list to identify the file.
  • who and when will be taking care to remove the dependencies so we can finish the PR and merge it?

Renamed audit file.

@MathisGD are you able to copy in any dependencies to remove the solmate and metamorpho libraries?

Copy link
Contributor Author

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Morpho dep has been removed/inlined. This is good to merge now imo.

Copy link
Contributor

@sunbreak1211 sunbreak1211 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hexonaut
Copy link
Contributor Author

Going to merge this. Additional cleanup can be done via separate PRs.

@hexonaut hexonaut merged commit 5684cd8 into master Mar 30, 2024
8 checks passed
@hexonaut hexonaut deleted the multisig-plan branch March 30, 2024 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.