test: After updating vaultFactory params, verify the change #10803
Conversation
There was a problem hiding this comment.
The assertion introduced in this PR is already covered in the subsequent test case, here.
However, I can understand if you find more appropriate to explicitly verify that the governed parameter was indeed updated in order to pass the economic committee can make governance proposal and vote on it test case.
| t.is( | ||
| // @ts-expect-error it's a record | ||
| vaultFactoryParamsAfter.current.ChargingPeriod.value, | ||
| 400n, |
There was a problem hiding this comment.
also: we talked about revising governanceDriver.waitForElection() to pay attention to which question is relevant.
There was a problem hiding this comment.
Are we sure it wasn't 400n before?
I've added a set-up step
also: we talked about revising governanceDriver.waitForElection() to pay attention to which question is relevant.
The governanceDriver remembers the deadline from proposeParamChange(), and verifies it in waitForElection().
Deploying agoric-sdk with
|
| Latest commit: |
ed61157
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://81d44cd1.agoric-sdk.pages.dev |
| Branch Preview URL: | https://cth-check-vf-params-after-up.agoric-sdk.pages.dev |
| // @ts-expect-error it's a record | ||
| vaultFactoryParamsBefore.current.ChargingPeriod.value === 400n |
There was a problem hiding this comment.
Rather than a ts-expect-error here at the point of usage, I suggest fixing the type at the point of declaration. A ts-expect-error there may or may not be necessary.
| // @ts-expect-error it's a record | ||
| vaultFactoryParamsAfter.current.ChargingPeriod.value, |
There was a problem hiding this comment.
likewise, I suggest fixing the decl of vaultFactoryParamsAfter
Chris-Hibbert
added
automerge:rebase
Chris-Hibbert
force-pushed
the
cth-check-vf-params-after-update
branch
from
1907728 to
4f52d8f
Compare
Description
We noticed that after changing params for the VaultFactory, the test verified that the vote had completed, but not that the param change was effective. For a while we suspected that the change didn't take, but a simple enhancement to the test showed that wasn't the case.
Security Considerations
None
Scaling Considerations
None
Documentation Considerations
None
Testing Considerations
Tests should verify the actual changes they make.
Upgrade Considerations
This was one red herring we chased while validating upgrade. A clearer test would have made that chase shorter.