test(a3p): extend governance test coverage for acceptance proposal #10555

Jorge-Lopes · 2024-11-22T22:54:27Z

closes: https://github.com/Agoric/BytePitchPartnerEng/issues/24
closes: #10708

Description

This PR has the objective of extending the a3p-integration acceptance proposal test coverage. More specifically, it intend to test that:

Configured EC governed parameters are intact following the contract upgrade
Governance proposals history is visible

The test plan to verify that the parameters governed by the economicCommittee are intact when the contract is upgraded is:

Propose a parameter change
Verify that the value was updated
Execute a null upgrade to the respective contract
Verify that the value kept the same

Although more contracts are governed by economicCommittee, the ones selected to be included in this test case were vaultFactory and provisionPool. A detailed discussion on the rationale behind this decision can be found at https://github.com/Agoric/BytePitchPartnerEng/issues/24

Note: The existing governance test and helper functions were refactored to allow the makeGovernanceDriver function to be contract agnostic, in order to be reused by vaultFactory, provisionPool and any other contract that is intend to be tested in the future.

The test plan is to verify that the vstorage node published.committees.Economic_Committee.latestQuestion displays the history of proposed Economic Committee parameter changes is:

Use the readHistory method to fetch all parameter change proposals from latestQuestion node, starting from block height 0.
Verify that returned value is not an empty list
Construct a list of expected parameters changes proposed by the economic committee prior to the governance test.
Compare the parameters keys of both lists match.

Security Considerations

No security considerations.

Scaling Considerations

The "Governance proposals history" test compares vstorage values against a hardcoded list, requiring manual updates to expectedParametersChanges for new proposals or tests that executes a EC governed param changes made prior to this test.

To prevent the halting the test when the developer does not update the hardcoded list, this test is only asserting that the value returned from the published.committees.Economic_Committee.latestQuestion vstorage node is not empty.
Then it will compare both lists and if any mismatches is found, it will:

print an error message notifying the mismatch.
print both current and expected changes proposals records

Note that this solution may risks oversight and could lead to latent issues, so I am open a implement a new design if desired.

Documentation Considerations

We propose adding a table to the z:acceptance README file. This table will:

List the parameters being modified during acceptance test execution.
Identify where and why these changes occur, providing developers with a clear understanding of parameter dependencies.

An issue was created to address this task.

Testing Considerations

The methods exported by makeVstorageKit at the @agoric/client-utils package, seems to display an unexpected behaviour, more specifically, when using the readFully method to query Vstorage node full history.
As a workaround, I updated the makeVStorage function at z:acceptance/test-lib/rpc.js to follow the same implementation as the multichain-testing/tools/batchQuery.js.
The rational behind this decision is explained in detail on the issue #10574

If desired, I can add these changes to @client-utils/src/vstorage.js in a future PR, contribute to the ongoing effort of reducing the dependency of the acceptance test-lib.

The order of execution of the acceptance tests had to be updated to make sure that the governance.test.js would run after the state-sync acceptance tests to prevent the behavior described here #10574

Upgrade Considerations

No upgrade considerations.

cloudflare-workers-and-pages · 2024-11-25T11:58:19Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`28b4884`
Status:	✅ Deploy successful!
Preview URL:	https://5910bae6.agoric-sdk.pages.dev
Branch Preview URL:	https://jorge-acceptance-econ-gov.agoric-sdk.pages.dev

View logs

… contract upgrade (#10638) closes: #10562 ## Description This PR introduces changes to the provisionPool contract to allow the overriding of the PerAccountInitialAmount parameter, governed by the Economic Committee, during a contract upgrade. To achieve the above functionality, this PR introduces the following changes: - The PerAccountInitialAmount can now be injected into the contract through privateArgs by defining governedParamOverrides. - The handleParamGovernance method from the governance package was updated to include an aditional optional argument called `overrides` so it can be passed to the makeParamManagerFromTerms to manage the governed parameter during contract upgrades. The core-eval proposal used to upgrade provisionPool was also updated to fetch the current value of PerAccountInitialAmount set on Vstorage and include it on the new privateArgs. If the governedParamOverrides is not provided to the privateArgs during the contract upgrade, the value of PerAccountInitialAmount will reset to the default one set on the contract terms. ### Security Considerations ### Scaling Considerations ### Documentation Considerations ### Testing Considerations The existing unit tests for the inter-protocol and vats packages, as well as the a3p-integration tests, have been verified to continue passing with these changes. Additionally, these changes were tested against the governance acceptance tests in the a3p-integration which ensures that governed parameter values remain intact after a contract upgrade, as well as testing that, if not included in the governedParamOverrides on the core-eval proposal, the value of PerAccountInitialAmount will reset to the default one set on the contract terms. To maintain the scope of this PR, that test is not included here, however, it will be introduced in PR #10555, which is planned to be opened once this is merged. ### Upgrade Considerations

Chris-Hibbert

Nice cleanup/enhancement of the governance driver.

a3p-integration/proposals/z:acceptance/test-lib/utils.js

Chris-Hibbert · 2024-12-13T19:39:51Z

a3p-integration/proposals/z:acceptance/test-lib/utils.js

+ */
+const getIncarnationFromDetails = async vatName => {
+  const matchingVats = await getDetailsMatchingVats(vatName);
+  const expectedVat = matchingVats.find(vat => vat.vatName === vatName);


should this fail if the name doesn't uniquely match a single vat? It seems like that could lead to surprising errors.

Thank you for bringing this to my attention.
An assert call was included to verify that expectedVat is not undefined.

That checks the difference between 0 and at least 1. I was asking whether we need to worry about the difference between 1 and more than 1. find will return the first instance. Do we need to worry about getIncarnationFromDetails returning a different vat than was intended because the vatName would have matched more than one vat?

I don't think that should be a concern.
Assuming that getDetailsMatchingVats will return all vats that can be related with the provided vat name, and that vat names are unique, we can be confident that the strict equality operator will will one be true for the single instance that we intend to get.

Chris-Hibbert · 2024-12-13T19:56:35Z

packages/vats/src/proposals/upgrade-vaultFactory-proposal.js

Please add @file comments here and in ./upgrade-vaults.js with mutual cross-references, and say that this one is intended to be generic and reusable, while the other does several things that are specific to upgrade-18.

Change request addressed

a3p-integration/proposals/z:acceptance/governance.test.js

Chris-Hibbert · 2024-12-13T21:46:57Z

a3p-integration/proposals/z:acceptance/test-lib/rpc.js

+/** @file copied from packages/agoric-cli */
+// TODO DRY in https://github.com/Agoric/agoric-sdk/issues/9109


I understand why we have to duplicate stuff from synthetic-chain. If we can import from @agoric/internal, why do we need to repeat code that's in agoric-cli?

I looked around and didn't find where these functions were copied from. Are they in agoric-sdk?

That comment was kept from the original rpc.js file, which end up being deleted in a previous PR and reintroduced on this one.
I updated the description to reflect the current implementation of rpc.js.

Chris-Hibbert

One remaining substantive question

Chris-Hibbert · 2024-12-16T17:08:52Z

a3p-integration/proposals/z:acceptance/test-lib/utils.js

+ */
+const getIncarnationFromDetails = async vatName => {
+  const matchingVats = await getDetailsMatchingVats(vatName);
+  const expectedVat = matchingVats.find(vat => vat.vatName === vatName);


That checks the difference between 0 and at least 1. I was asking whether we need to worry about the difference between 1 and more than 1. find will return the first instance. Do we need to worry about getIncarnationFromDetails returning a different vat than was intended because the vatName would have matched more than one vat?

packages/vats/src/proposals/upgrade-vaultFactory-proposal.js

…t agnostic

rel #10574

rel: #10574

Co-authored-by: Chris Hibbert <[email protected]>

a3p-integration/proposals/z:acceptance/test-lib/governance.js

mergify · 2024-12-17T09:20:10Z

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #10555 has been dequeued. The pull request has been synchronized by a user.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

refs: #10708

anilhelvaci · 2024-12-17T11:33:17Z

@Mergifyio requeue

mergify · 2024-12-17T11:33:20Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

Jorge-Lopes added the force:integration Force integration tests to run on PR label Nov 22, 2024

This was referenced Nov 25, 2024

Ability to override the ProvisionPool governedParam PerAccountInitialAmount #10562

Closed

Failed calls to readFully method of client-utils package #10563

Closed

Jorge-Lopes force-pushed the jorge/acceptance-econ-gov branch from 7532428 to 3de052b Compare November 26, 2024 10:37

Jorge-Lopes closed this Nov 28, 2024

Jorge-Lopes force-pushed the jorge/acceptance-econ-gov branch from 3de052b to 1dd4589 Compare November 28, 2024 19:53

Jorge-Lopes removed the force:integration Force integration tests to run on PR label Nov 28, 2024

Jorge-Lopes reopened this Dec 2, 2024

Jorge-Lopes force-pushed the jorge/acceptance-econ-gov branch from 20f9781 to 89e4be7 Compare December 2, 2024 15:01

Jorge-Lopes added the force:integration Force integration tests to run on PR label Dec 2, 2024

This was referenced Dec 5, 2024

feat(provisionPool): allow overriding PerAccountInitialAmount during contract upgrade #10594

Closed

chore(provisionPool): allow overriding PerAccountInitialAmount during contract upgrade #10638

Merged

Jorge-Lopes force-pushed the jorge/acceptance-econ-gov branch 4 times, most recently from 649cde3 to 72cbbc6 Compare December 11, 2024 19:52

Jorge-Lopes force-pushed the jorge/acceptance-econ-gov branch from 1b456eb to 77f5205 Compare December 13, 2024 16:06

Jorge-Lopes marked this pull request as ready for review December 13, 2024 16:09

Jorge-Lopes requested a review from a team as a code owner December 13, 2024 16:09

Jorge-Lopes requested a review from AgoricTriage December 13, 2024 16:09

Jorge-Lopes force-pushed the jorge/acceptance-econ-gov branch 2 times, most recently from d6e4578 to 842fa8a Compare December 13, 2024 18:01

Jorge-Lopes requested a review from Chris-Hibbert December 13, 2024 18:57

Chris-Hibbert reviewed Dec 13, 2024

View reviewed changes

Jorge-Lopes requested a review from Chris-Hibbert December 16, 2024 15:05

Chris-Hibbert reviewed Dec 16, 2024

View reviewed changes

Jorge-Lopes requested a review from Chris-Hibbert December 16, 2024 18:03

Chris-Hibbert approved these changes Dec 16, 2024

View reviewed changes

Jorge-Lopes added the automerge:rebase Automatically rebase updates, then merge label Dec 17, 2024

Jorge-Lopes mentioned this pull request Dec 17, 2024

z:acceptance governance fails/flakes: No quorum #10708

Closed

Jorge-Lopes and others added 10 commits December 17, 2024 09:12

chore(vaultFactory): add core-eval for null upgrade vaultFactory

1eb858d

chore(a3p): add new submissions from governance acceptance tests

662fa2c

chore(governance):Refactor makeGovernanceDriver methods to be contrac…

75c8a12

…t agnostic

fix(a3p): change test execution order

c9bd9a4

rel #10574

test(a3p): extend governance testing coverage

4b660fd

chore(a3p): refactor makeVStorage to be aligned with batchQuery.js

938d615

rel: #10574

test(a3p): extend governance test with proposals history

dccf7bc

chore(vaultFactory): include insightful comments

dc7388c

fix(a3p): minor fixes to governance test and helpers

ecddebd

fix(vaultFactory): fix proposal description

bc1f87a

Co-authored-by: Chris Hibbert <[email protected]>

Jorge-Lopes force-pushed the jorge/acceptance-econ-gov branch from 02c201f to bc1f87a Compare December 17, 2024 09:12

anilhelvaci reviewed Dec 17, 2024

View reviewed changes

a3p-integration/proposals/z:acceptance/test-lib/governance.js Outdated Show resolved Hide resolved

fix(a3p): remove duplicated await

cc4c1a2

chore: switch using waitForElectionResult, eliminate flakiness

28b4884

refs: #10708

mergify bot merged commit 9835fb0 into master Dec 17, 2024
82 checks passed

mergify bot deleted the jorge/acceptance-econ-gov branch December 17, 2024 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(a3p): extend governance test coverage for acceptance proposal #10555

test(a3p): extend governance test coverage for acceptance proposal #10555

Jorge-Lopes commented Nov 22, 2024 •

edited by anilhelvaci

Loading

cloudflare-workers-and-pages bot commented Nov 25, 2024 •

edited

Loading

Chris-Hibbert left a comment

Chris-Hibbert Dec 13, 2024

Jorge-Lopes Dec 16, 2024

Chris-Hibbert Dec 16, 2024

Jorge-Lopes Dec 16, 2024

Chris-Hibbert Dec 13, 2024

Jorge-Lopes Dec 16, 2024

Chris-Hibbert Dec 13, 2024

Jorge-Lopes Dec 16, 2024

Chris-Hibbert left a comment

Chris-Hibbert Dec 16, 2024

mergify bot commented Dec 17, 2024

anilhelvaci commented Dec 17, 2024

mergify bot commented Dec 17, 2024

		/** @file copied from packages/agoric-cli */
		// TODO DRY in https://github.com/Agoric/agoric-sdk/issues/9109

test(a3p): extend governance test coverage for acceptance proposal #10555

test(a3p): extend governance test coverage for acceptance proposal #10555

Conversation

Jorge-Lopes commented Nov 22, 2024 • edited by anilhelvaci Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Nov 25, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Dec 17, 2024

anilhelvaci commented Dec 17, 2024

mergify bot commented Dec 17, 2024

❌ This pull request head commit has not been previously disembarked from queue.

Jorge-Lopes commented Nov 22, 2024 •

edited by anilhelvaci

Loading

cloudflare-workers-and-pages bot commented Nov 25, 2024 •

edited

Loading