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

feat: separate vaults launch from platform #7563

Closed
wants to merge 21 commits into from
Closed

Conversation

dckc
Copy link
Member

@dckc dckc commented May 1, 2023

closes: #5819 (or at least: refs)

The code still needs some rebasing / fixup, but I works.

@warner do you want to review this? feel free to take yourself out of the list.

Description

  • remove vaults proposal from devnet bootstrap config

  • take governance committee, oracles, out of vaults proposal

  • take stakeFactory out of vaults proposal

  • add initial collateral type

  • prune dead code from packages/inter-protocol/scripts/init-core.js, packages/inter-protocol/src/proposals/core-proposal.js

  • review install Inter protocol defi contracts after boostrap using publishBundle #5819 etc. for configuration details to take care of

Security Considerations

nothing new, I don't think

Scaling Considerations

  • verify that the largest crank is smaller than the block limit, suggesting that work would be broken up between blocks expectedly

Documentation Considerations

The proposal to launch vaults should come with a community discussion post.

See also

Testing Considerations

  • bootstrap test
  • test that initial collateral type is installed, by checking the metrics for list of collaterals

TODO:

  • wire oracles
  • test that vaults are fully functional by opening a vault?
  • test that we start econCommitteeCharter only once (feat: label contract vats #7426 (comment))
  • test enacting the proposal in a live chain; in particular

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

A first pass. The testing plan LGTM. Might break loadgen? if so you might want to force integration after getting this synced again with master

if (e.code !== 'ENOENT') {
throw e;
}
// FIXME: We could take a hard dependency on `tmp` here, but is it worth it?
Copy link
Member

Choose a reason for hiding this comment

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

probably not worth the NPM dependency but we do have Node and could use mkdtemp

Copy link
Member Author

Choose a reason for hiding this comment

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

that file isn't actually in this PR; it's an artifact of stacking this on mfig-cli-run-no-solo

@@ -355,7 +355,7 @@ export const startRewardDistributor = async ({
bankManager,
vaultFactoryKit,
periodicFeeCollectors,
stakeFactoryKit,
// stakeFactoryKit,
Copy link
Member

Choose a reason for hiding this comment

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

simply delete for now

/**
* Introduce charter to governed creator facets.
*
* XXX make this the responsibility of startVaultFactory, startReserve, etc.?
Copy link
Member

Choose a reason for hiding this comment

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

I think so. why not?

Copy link
Member Author

Choose a reason for hiding this comment

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

just time.

return { permit, script, bundles };
};

test.serial('make vaults launch proposal', async t => {
Copy link
Member

Choose a reason for hiding this comment

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

yay bootstrap test!

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Looking good so far!

@@ -171,13 +171,13 @@ export const makeBootstrap = (
const { root } = vat;
const decodedArgs = args.map(decodePassable);
const result = await E(root)[methodName](...decodedArgs);
return encodePassable(result);
return encodePassable(harden(result));
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

@mhofman mhofman added the force:integration Force integration tests to run on PR label May 4, 2023
@mhofman
Copy link
Member

mhofman commented May 4, 2023

Updated the PR with some cleanups around proposal builder, installing the ATOM collateral, measuring per crank computrons. All tests pass, but the PR still needs some rebasing / fixups as I mostly kept the original WIP commits.

@turadg turadg mentioned this pull request May 4, 2023
@dckc
Copy link
Member Author

dckc commented May 4, 2023

@mhofman thanks for polishing this up, but as noted in #5819 (comment) , this is no longer the plan.

If you added anything along the way that should be kept, please put it in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

install Inter protocol defi contracts after boostrap using publishBundle
4 participants