search

Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
303 stars 191 forks source link

Code Review - Crabble core-eval #8464

Closed aj-agoric closed 6 months ago

aj-agoric commented 8 months ago

What is the Problem Being Solved?

Task: Code review the core-eval which will be used to deploy the contract. Purpose: This allows verification that the permissions asked for in the core-eval are necessary. This allows verification that the bundles being installed on chain are the bundles that have been reviewed. Instructions: OpCo engineers to arrange review with partner via Slack.

Description of the Design

Security Considerations

Scaling Considerations

Test Plan

Upgrade Considerations

dckc commented 7 months ago

The current proposal-permit.json includes several very powerful capabilities:

This includes authority to upgrade any contract, mint IST, etc.

Looking at core-eval.js, this seems to be motivated by lack of...

dckc commented 7 months ago

namesByAddressAdmin shouldn't be necessary; namesByAddress should suffice, but for...

dckc commented 6 months ago

As of crabble PR 155 / 0d0759e, the code to start the Crabble contract does not get the powerful authorities below. The approach is to have 1 swingset-core-eval governance proposal with 2 permit/script pairs:

  1. fixes #8330 by building startMyGovernedUpgradable out of the powerful capabilities
  2. starts the Crabble contract by using startMyGovernedUpgradable

The fix for #8330 included in 0d0759e is largely based on the #8589 draft. @turadg I presumed that you're satisfied with the result when I approved crabble PR 155; if not, please let us know.

The current proposal-permit.json includes several very powerful capabilities:

  • contractKits
  • governedContractKits
  • instancePrivateArgs

This includes authority to upgrade any contract, mint IST, etc.

dckc commented 6 months ago

In discussion with @turadg , we've come up with a different design we prefer for #8330: https://github.com/Agoric/agoric-sdk/pull/8589#pullrequestreview-1775823084

dckc commented 6 months ago

Granting authority to start a bespoke contract governor is something I didn't look at closely enough earlier. In discussion of #8330, @turadg noticed that the crabble governor doesn't support replacing the electorate. We've submitted a PR (https://github.com/CrabblePitch/crabbleProtocol/pull/161) to address that.

dckc commented 6 months ago

OK, this is done. PR 161 was merged in ba318e3