Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(a3p): create a3p test for replace electorate core eval #10241

Merged
merged 6 commits into from
Oct 24, 2024

Conversation

frazarshad
Copy link
Contributor

@frazarshad frazarshad commented Oct 8, 2024

closes: #10138
closes: #10185
closes: #10258

Description

A3P tests for the replace electorate core eval. also adds the replace-electorate-core.js script to upgrade.go since it will now be a part of the chain halting upgrade

new tests are as follows:

  • adds new n:replace-electorate which tests for acceptance of new invitations
  • adds params governance proposal and voting tests to z:acceptance

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@frazarshad frazarshad self-assigned this Oct 8, 2024
Copy link

cloudflare-workers-and-pages bot commented Oct 8, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 56f8b3c
Status: ✅  Deploy successful!
Preview URL: https://a65be0fe.agoric-sdk.pages.dev
Branch Preview URL: https://fraz-replace-committee-a3p.agoric-sdk.pages.dev

View logs

@frazarshad frazarshad force-pushed the rs-faraz-replace-charter branch from ffce1a7 to aef84d0 Compare October 8, 2024 18:05
@@ -0,0 +1,5 @@
export const waitUntil = async waitTime => {
Copy link
Member

@dckc dckc Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const waitUntil = async waitTime => {
export const waitUntil = async (waitTime, { setTimeout = globalThis.setTimeout, now = Date.now } = {}) => {

see

-1,
fromCapData,
);
await waitUntil(latestQuestion.closingRule.deadline);
Copy link
Member

@dckc dckc Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await waitUntil(latestQuestion.closingRule.deadline);
const { setTimeout } = globalThis;
const { now } = Date;
await waitUntil(latestQuestion.closingRule.deadline, { setTimeout, now });

@@ -0,0 +1,5 @@
export const waitUntil = async waitTime => {
while (Math.floor(Date.now() / 1000) < waitTime) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
while (Math.floor(Date.now() / 1000) < waitTime) {
while (Math.floor(now() / 1000) < waitTime) {

@rabi-siddique rabi-siddique force-pushed the rs-faraz-replace-charter branch from 6a3afcb to 78581b1 Compare October 10, 2024 05:48
Base automatically changed from rs-faraz-replace-charter to master October 10, 2024 06:24
@frazarshad frazarshad force-pushed the fraz/replace-committee-a3p branch from 5082910 to 519de18 Compare October 10, 2024 06:33
@frazarshad frazarshad added the force:integration Force integration tests to run on PR label Oct 14, 2024
@frazarshad frazarshad requested a review from dckc October 14, 2024 10:58
@frazarshad frazarshad marked this pull request as ready for review October 14, 2024 10:58
@frazarshad frazarshad requested a review from a team as a code owner October 14, 2024 10:58
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of good stuff here. But the EC changes should take place as part of n:upgrade-next; see #10258 .

The z:acceptance stuff could probably land as its own PR. I would expect it to work before or after the change of membership. Hm... maybe that would complicate things.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several parts that I don't understand.

Comment on lines 6 to 9
await agops.ec(
'charter',
'--send-from',
GOV1ADDR,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only gov1? Won't we need the others to accept theirs too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the config for A3P_INTEGRATION seems to have only one member in the committee. hence only one call to accept the invitation

voterAddresses: {
gov1: 'agoric1ee9hr0jyrxhy999y755mp862ljgycmwyp4pl7q',
},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odd. ok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages/synthetic-chain/src/lib/constants.js has GOV1ADDR, GOV2ADDR, and GOV3ADDR.

packages/builders/scripts/inter-protocol/updatePriceFeeds.js has all three.

Where did you find a short list?

Comment on lines 26 to 29
export const governanceDriver = await makeGovernanceDriver(
fetch,
networkConfig,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Injecting fetch and networkConfig should be left to test scripts and not in a module like this.

see OCap Discipline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.
also isnt the walletUtils object in this file breaking this principle?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is waitUntil, for that matter.

@frazarshad frazarshad requested a review from dckc October 21, 2024 16:44
const config = configurations[variant];
/** @type {import('@agoric/deploy-script-support/src/externalTypes.js').CoreEvalBuilder} */
export const defaultProposalBuilder = async ({ publishRef, install }, opts) => {
const config = configurations[opts.variant];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! I missed this earlier. Now I get it.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it addresses the critical stuff. Good work!

I leave updating walletUtils and waitUntil to your discretion.

@Chris-Hibbert
Copy link
Contributor

I'd like to wait to review till @dckc's requested changes are in. Please ping me at that point.

@frazarshad
Copy link
Contributor Author

@Chris-Hibbert all requested changes from @dckc are done

Comment on lines 6 to 9
await agops.ec(
'charter',
'--send-from',
GOV1ADDR,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages/synthetic-chain/src/lib/constants.js has GOV1ADDR, GOV2ADDR, and GOV3ADDR.

packages/builders/scripts/inter-protocol/updatePriceFeeds.js has all three.

Where did you find a short list?

getLastUpdate(address, { readLatestHead }),
),
);
voteUpdates.forEach(voteUpdate => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint flags this with Prefer for...of instead of Array.forEach. Is there any reason to prefer forEach here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not particularly, just missed it. updating

Comment on lines 87 to 105
var variant string

switch validUpgradeName(upgradeName) {
case "UNRELEASED_A3P_INTEGRATION":
variant = "A3P_INTEGRATION"
case "UNRELEASED_main":
variant = "MAINNET"
case "UNRELEASED_devnet":
variant = "DEVNET"
// Noupgrade for this version.
case "UNRELEASED_BASIC":
}
variantJson, err := json.Marshal(variant)
if err != nil {
return nil, err
}
var result strings.Builder
err = t.Execute(&result, map[string]any{
"variantJson": string(variantJson),
})
if err != nil {
return nil, err
}
jsonStr := result.String()
jsonBz := []byte(jsonStr)
if !json.Valid(jsonBz) {
return nil, fmt.Errorf("invalid JSON: %s", jsonStr)
}
proposal := vm.ArbitraryCoreProposal{Json: jsonBz}
return vm.CoreProposalStepForModules(proposal), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to need to do all of this for the priceFeed upgrade. Can it be defined so it can be shared rather than requiring me to copy it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I build on top of this for #10317

@frazarshad
Copy link
Contributor Author

Where did you find a short list?

@Chris-Hibbert new A3P_INTEGRATION committee only has one member

voterAddresses: {
gov1: 'agoric1ee9hr0jyrxhy999y755mp862ljgycmwyp4pl7q',
},

dckc
dckc previously requested changes Oct 22, 2024
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a3p config needs gov4

const config = configurations[variant];
/** @type {import('@agoric/deploy-script-support/src/externalTypes.js').CoreEvalBuilder} */
export const defaultProposalBuilder = async ({ publishRef, install }, opts) => {
const config = configurations[opts.variant];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be sure to add gov4 in the a3p configuration.

for (const address of governanceAddresses) {
/** @type {any} */
const voteWallet = await readLatestHead(
`published.wallet.${GOV1ADDR}.current`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`published.wallet.${GOV1ADDR}.current`,
`published.wallet.${address}.current`,

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

presuming that the emerynet deployment uses the UNRELEASED_devnet config, this looks great!

@frazarshad frazarshad force-pushed the fraz/replace-committee-a3p branch from 56f8b3c to f406d25 Compare October 24, 2024 15:35
@frazarshad frazarshad added the automerge:rebase Automatically rebase updates, then merge label Oct 24, 2024
@mergify mergify bot merged commit a67502b into master Oct 24, 2024
87 checks passed
@mergify mergify bot deleted the fraz/replace-committee-a3p branch October 24, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
3 participants