refactor(run-protocol): vaultDirector to multifacted vobj #5116
Conversation
| !collateralTypes.has(collateralBrand), | ||
| `Collateral brand ${collateralBrand} has already been added`, | ||
| ); | ||
| // TODO put on machineFacet for use again in publicFacet |
There was a problem hiding this comment.
what I went with is defining this at top scope so it can be shared.
thoughts on that vs giving it a primary facet and letting another reexport?
There was a problem hiding this comment.
both approaches seem fine where appropriate. This seems sufficient for this use. I can think of other places where we might want a facet. (I'm considering that for the AMM's singlePool, but that has more justification to be a facet.)
| }; | ||
|
|
||
| /** @type {import('@agoric/vat-data/src/types').FunctionsPlusContext<VaultFactory>} */ | ||
| const machineBehavior = { |
There was a problem hiding this comment.
the functions within this behavior could be at module-scope. That's how @FUDCo had in the initial defineKind-ization of vault.js. I propose a style of defining behavior objects with the methods inlined so it's easier to navigate.
There was a problem hiding this comment.
I had originally had the behavior functions in vault.js at module scope simply because it was the most similar to what had been there before. @turadg asked me to rewrite them to be inline and I think I like the inline style better in that case. I don't know how it translates to this file however.
| mintAndReallocate, | ||
| burnDebt, | ||
| }); | ||
| const factoryPowers = Far('vault factory powers', { |
There was a problem hiding this comment.
will defining a far per vault manager pose any problems for durability @FUDCo ?
There was a problem hiding this comment.
On general principles it should not be a problem, but the key question is: will you be able to reconstruct the Far from the durable state after resurrection? (I can't discern the answer to that myself that without digging into this code considerably beyond my feeble understanding of what it's doing.) If you can, then it should be no problem; you just need to make sure your startup flow actually does it. Otherwise you'll either need to (a) capture more info in the durable part so that you can reconstruct it or (b) make it durable itself. I don't have an opinion which is more right, as it would depend on the particulars, other than the general advice to take whichever course causes you the least pain.
|
|
||
| const machineFacet = Far('vaultFactory machine', machineBehavior); | ||
| // XXX accessors for tests |
There was a problem hiding this comment.
once we have a way to instantiate a vobj from a state fixture, we don't need these methods. we can just inspect the state after the operations.
turadg
force-pushed
the
4345-virtualize-vaultDirector
branch
from
8305d29
to
1c87554
Compare
turadg
force-pushed
the
4345-virtualize-vaultDirector
branch
from
1c87554
to
756e7f2
Compare
turadg
force-pushed
the
4345-virtualize-vaultDirector
branch
from
756e7f2
to
90ad0e9
Compare
| !collateralTypes.has(collateralBrand), | ||
| `Collateral brand ${collateralBrand} has already been added`, | ||
| ); | ||
| // TODO put on machineFacet for use again in publicFacet |
There was a problem hiding this comment.
both approaches seem fine where appropriate. This seems sufficient for this use. I can think of other places where we might want a facet. (I'm considering that for the AMM's singlePool, but that has more justification to be a facet.)
closes: #4345
Description
Turns the new vaultDirector into a virtual object.
Security Considerations
State storage to to disk.
Documentation Considerations
--
Testing Considerations
Refactor so it maintains existing tests AMAP, though I did run into a test bug I fixed in 75e3e7
It took me too long to figure out 1c87554 so I think we need more test coverage or some static type validation of what is returned by facets. The problem wasn't detected in unit tests, only integration tests, and was much harder to debug there.