5200 upgradable contractGovernor #7657
Conversation
There was a problem hiding this comment.
the Far change looks like a breaking contract api change. I'd like to know if I'm on target about that before approving.
It's probably worth waiting on a review by @Chris-Hibbert too
| zoe, | ||
| governedInstance, |
There was a problem hiding this comment.
these weren't used? how did that pass lint?
There was a problem hiding this comment.
because the subsequent params were used, they had to stay there for backwards compatibility
| createdFilterQuestion: b => voteCounters.has(b), | ||
| createdQuestion: b => voteCounters.has(b), |
There was a problem hiding this comment.
is this a breaking contract API change? Is it required?
There was a problem hiding this comment.
an internal refactor: refactor(governor): consistent createdQuestion
There was a problem hiding this comment.
good question. I don't really know. I do know that what is returned does not exposed outside the contractGovernorKit. But given the time pressure I'm content to strike the change.
If you approve contingent on that I'll omit it in the rebase I have to do.
There was a problem hiding this comment.
I looked around and i don't see it escaping, so I agree it's internal.
| * @property {import('@agoric/time/src/types').TimerService} timer | ||
| * @property {import('@ agoric/time/src/types').TimerService} timer |
| electionManager: zcf.getInstance(), | ||
| }); | ||
|
|
||
| trace('starting governedContractInstallation'); |
There was a problem hiding this comment.
add governedContractLabel to this trace?
| return invitationAmount.value[0].instance; | ||
| }; | ||
| // trace('initiating start()'); | ||
| // Awaiting this prevents vat restart |
There was a problem hiding this comment.
So this is not commenting out dead code but telling future maintainers "you may think to do it this way; don't." right?
| @@ -0,0 +1,233 @@ | |||
| import { Fail } from '@agoric/assert'; | |||
There was a problem hiding this comment.
I think the tests are better at judging whether the logic was preserved when moving this to a new file in exo form than I would be, so I'm just skimming.
There was a problem hiding this comment.
I did more than skim. I don't see any errors or omissions. I think it's all here.
| const upgradeResult = await EV(vfAdminFacet).restartContract(privateArgs); | ||
| t.deepEqual(upgradeResult, { incarnationNumber: 1 }); | ||
| t.like(t.context.readCollateralMetrics(0), keyMetrics); // unchanged | ||
| }); | ||
|
|
||
| test.serial('restart contractGovernor', async t => { |
There was a problem hiding this comment.
I'd like to hear from @Chris-Hibbert on this, but I'm ok if we don't.
| import { ParamChangesQuestionDetailsShape } from './typeGuards.js'; | ||
|
|
||
| const { Fail } = assert; | ||
|
|
||
| const trace = makeTracer('CGov', false); | ||
| const trace = makeTracer('CGov'); |
There was a problem hiding this comment.
revert dropping false.
| @@ -0,0 +1,233 @@ | |||
| import { Fail } from '@agoric/assert'; | |||
There was a problem hiding this comment.
I did more than skim. I don't see any errors or omissions. I think it's all here.
turadg
force-pushed
the
5200-upgradable-contractGovernor
branch
from
964504f
to
6a6d751
Compare
refs: #5200
Description
This makes the
contractGovernorcontract upgradable. It's not yet fully durable because some facets aren't durable and the open questions can't be validated after a contract restart/upgrade, so I'm leaving #5200 open.It does satisfy the requirement that each governor contract vat be upgradable,
Security Considerations
Authority from baggage
Scaling Considerations
Should lower memory pressure slightly
Documentation Considerations
Little change to API. The
voteOnApiInvocationis now async, so that it can lazily query the remote governed contract for its API methods.Testing Considerations
Existing tests pass. Bootstrap test of vaults upgrade now upgrades the vaultFactory's governor.