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

Builder Script for Replacing Committee and Charter #10161

Closed
wants to merge 4 commits into from

Conversation

rabi-siddique
Copy link
Contributor

@rabi-siddique rabi-siddique commented Sep 26, 2024

refs: #10136 #10135

Description

The PR creates a builder script for the core eval for replacing the committee and charter in #10105

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

I haven't found much about builder scripts in our documentation. Mainly took pointers from #8087 shared by Dan.
If appropriate, we can document the process detailing what it is and how it works.

Testing Considerations

I tested it with a3p mainly via this bash script I created:
https://gist.github.com/rabi-siddique/5dc08dd3d167c4a1ecbc912d42fee68a

Upgrade Considerations

n/a

@rabi-siddique rabi-siddique changed the base branch from master to rs-ec-changes September 26, 2024 09:27
Copy link

cloudflare-workers-and-pages bot commented Sep 26, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: a038d6e
Status: ✅  Deploy successful!
Preview URL: https://69114843.agoric-sdk.pages.dev
Branch Preview URL: https://rs-builder-script-for-ec-cha.agoric-sdk.pages.dev

View logs

@rabi-siddique rabi-siddique force-pushed the rs-builder-script-for-ec-changes branch from e195a64 to ee927b8 Compare September 26, 2024 11:40
@rabi-siddique rabi-siddique force-pushed the rs-builder-script-for-ec-changes branch from 8a7f6b6 to a038d6e Compare September 26, 2024 14:04
await startNewEconomicCommittee(permittedPowers);
await startNewEconCharter(permittedPowers);
await addGovernorsToEconCharter(permittedPowers);
await handlehighPrioritySendersList(permittedPowers);
// await handlehighPrioritySendersList(permittedPowers);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commented line 338. Because the shutdown definition assumes that committee and charter contracts have shutdown method in their creatorFacet.

Commented 343 too. Although it's not required if I've updated the wallet addresses on line 5 in runConfig object. Wallet addresses should be based on a3p.

'@agoric/inter-protocol/src/econCommitteeCharter.js',
'../bundles/bundle-econCommitteeCharter.js',
),
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having developed the builder script by referencing other examples in our repo, I have some questions around:

        ...opts,
        economicCommitteeRef: publishRef(
          install(
            '@agoric/governance/src/committee.js',
            '../bundles/bundle-committee.js',
          ),
        ),
        economicCharterRef: publishRef(
          install(
            '@agoric/inter-protocol/src/econCommitteeCharter.js',
            '../bundles/bundle-econCommitteeCharter.js',
          ),
        ),
  • What are the roles of economicCommitteeRef and economicCharterRef? Specifically, what are these references used for in the script?
  • Regarding the install function, what does the second parameter represent? Is it indicating the directory where the bundles are located, or does it serve another purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this section of code responsible for generating new bundles? If yes, I reckon we don't need to include:

    economicCommitteeRef: publishRef(
          install(
            '@agoric/governance/src/committee.js',
            '../bundles/bundle-committee.js',
          ),
        ),
        economicCharterRef: publishRef(
          install(
            '@agoric/inter-protocol/src/econCommitteeCharter.js',
            '../bundles/bundle-econCommitteeCharter.js',
          ),

That's something we plan to do in 2 different core evals(Maybe I'll add that in #10151 and #10150) which updates the creatorFacet of committee and charter contracts with the relevant methods after our design for it is finalized.

@rabi-siddique rabi-siddique requested a review from dckc September 26, 2024 14:26
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 is good progress.

The tests need to be included in the PR, though. They need to run in ci.

I strongly suggest making one PR that matches the scope of #10136 -- including the test plan -- and another that matches #10137 (#10135 is a user story, but building it consists of #10134 and #10137).

I much prefer to see #10134 addressed first, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants