Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
328 stars 207 forks source link

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

Closed Jorge-Lopes closed 1 day ago

Jorge-Lopes commented 1 week ago

closes: (https://github.com/Agoric/BytePitchPartnerEng/issues/24

Description

This PR has the purpose of extending the a3p-integration acceptance proposal test coverage. More specifically, add two additional test cases to governance.test.js:

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

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

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

Security Considerations

In the test case Configured EC parameters are intact following the contract upgrade, it became noticeable that after executing a null upgrade to the provisionPool contract using the core-eval packages/builders/scripts/vats/upgrade-provisionPool.js, the governed parameter PerAccountInitialAmount will reset to the original value.

The contract does not seem to provide a way to override the governedParams via privateArgs, which could result in undesired behavior in the future.

On the other side, VaultFactory does allow the overridden of the governedParams by providing the directorParamOverrides to the privateArgs

Scaling Considerations

The test case Governance proposals history is visible by comparing the Vstorage values against a hardcoded list. This results in the need to manually update the expectedParametersChanges list every time a new proposal or acceptance test is executed before this test case.

This is explained with in-line comments on the test file to help any developer facing a failed test.

Another approach could be to test that the Vstorage node is not empty and then, make a comparison that when resulted in a mismatch, a message will be logged to the console to notify the developer. However, this last approach can be easily overlooked.

Documentation Considerations

Testing Considerations

To verify that the contract null upgrade was successful, I am using the retryUntilCondition method pooling the vat incarnation until it increases. However, I noticed that when providing the vatName with the contract name, this can lead to multiple vats, which can be confirmed with the method getDetailsMatchingVats, so I decided to add the complete vatName.

My concern is if the vatName I am provided can be susceptible to changes and could result in a future conflict or failing test.

The methods exported by makeVstorageKit at the @agoric/client-utils package seem to break when using methods to query Vstorage such as readFully. To provide a workaround I updated the z:acceptance/test-lib/rpc.js to solve the issue being triggered.

Upgrade Considerations

cloudflare-workers-and-pages[bot] commented 4 days ago

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3de052b
Status: ✅  Deploy successful!
Preview URL: https://aab1e896.agoric-sdk.pages.dev
Branch Preview URL: https://jorge-acceptance-econ-gov.agoric-sdk.pages.dev

View logs