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

Documentation for the CoreEval feature #799

Merged
merged 18 commits into from
Jun 20, 2023
Merged

Documentation for the CoreEval feature #799

merged 18 commits into from
Jun 20, 2023

Conversation

Tyrosine22
Copy link
Contributor

@Tyrosine22 Tyrosine22 commented May 8, 2023

Here's the initial pass at CoreEval documentation.

BTW, some of the changes which originally appeared in this PR have been moved to the revisedgettingstarted PR (#811).

The gist with the output from the initial set of console commands is located here: https://gist.github.com/Tyrosine22/6da2367ae8897736953c2f1286164eed

The gist with the output from the final set of console commands is located here:
https://gist.github.com/Tyrosine22/12ba3222f15926f25eee30ce86be33f2

@Tyrosine22 Tyrosine22 requested a review from ivanlei May 8, 2023 19:13
@Tyrosine22 Tyrosine22 changed the title Newcoreeval Documentation for the CoreEval feature May 8, 2023
Copy link
Contributor

@ivanlei ivanlei left a comment

Choose a reason for hiding this comment

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

Can you please make requested changes and reply to questions.
How were the example shell commands tested? Can you add to the PR description a block of console output showing each example shell command being run and the successful output of them?

main/guides/coreeval/permissions.md Outdated Show resolved Hide resolved
main/guides/coreeval/permissions.md Outdated Show resolved Hide resolved
main/guides/coreeval/permissions.md Outdated Show resolved Hide resolved
@@ -0,0 +1,59 @@
# Code the Proposal

You will need to write a proposal script that requests storageNode powers and passes them to the contract via privateArgs. For example, [here](https://github.com/Agoric/agoric-sdk/blob/master/packages/inter-protocol/test/psm/gov-add-psm.js) is the proposal we created for the PSM contract:
Copy link
Contributor

Choose a reason for hiding this comment

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

question:
The script below doesn't make use of storageNode. What is it meant to highlight or explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote the text blurbs at the top of both this page & the README.md page. Take a look at both, please.

main/guides/coreeval/permissions.md Show resolved Hide resolved
@@ -0,0 +1,89 @@
# Coding Permissions

Write a json file with the permissions that the proposal will need. For example, [here](https://github.com/Agoric/agoric-sdk/blob/master/packages/inter-protocol/test/psm/gov-add-psm-permit.json) is the proposal Agoric created for the PSM contract:
Copy link
Contributor

Choose a reason for hiding this comment

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

comment:
The example is somewhat out of date with the code on master. Probably not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely think that's fine. The additional changes are contract-specific. However, one of the changes appears to be upgrade-related, so after I document the upgrade API, I want to return to this section, update the permissions page, and discuss the additional properties.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 11, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: eb90ae7
Status: ✅  Deploy successful!
Preview URL: https://393c6a1c.documentation-7tp.pages.dev
Branch Preview URL: https://newcoreeval.documentation-7tp.pages.dev

View logs

@Tyrosine22 Tyrosine22 requested a review from ivanlei May 11, 2023 20:54
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Looking good so far! I have some suggestions, and will defer approval to another reviewer.

@@ -0,0 +1,12 @@
# Injecting Code to Agoric Testnet

Agoric facilitates safely injecting code to the Agoric testnet, usually for the purpose of running a contract and modifying it based on the result of a governance vote. To do so, we submit a proposal using cosmos-sdk level API ([swingset.CoreEval](https://community.agoric.com/t/blder-dao-governance-using-arbitrary-code-injection-swingset-coreeval/99)). The proposal runs a script that runs the contract, and possibly runs an additional script, depending on the result of a governance vote.
Copy link
Member

Choose a reason for hiding this comment

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

I took another stab at clarifying the last sentence, please consider with a grain of salt:

Suggested change
Agoric facilitates safely injecting code to the Agoric testnet, usually for the purpose of running a contract and modifying it based on the result of a governance vote. To do so, we submit a proposal using cosmos-sdk level API ([swingset.CoreEval](https://community.agoric.com/t/blder-dao-governance-using-arbitrary-code-injection-swingset-coreeval/99)). The proposal runs a script that runs the contract, and possibly runs an additional script, depending on the result of a governance vote.
Agoric facilitates safely injecting code to the Agoric testnet, usually for the purpose of running a contract and modifying it based on the result of a governance vote. To do so, we submit a proposal using cosmos-sdk level API ([swingset.CoreEval](https://community.agoric.com/t/blder-dao-governance-using-arbitrary-code-injection-swingset-coreeval/99)). If the proposal passes its governance vote, its JS code is run with ocaps extracted using the proposal's permit list, which the code can combine to perform privileged tasks.

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 sentence has been changed to your suggestion.

* **board**: Grants write access to the board name service. This permission is somewhat specific to the PSM contract.
* **chainStorage**: Grants write access to the chain storage node, which is required when running `agd query` commands. Thus, most contracts will need access to this.
* **zoe**: When this permission is set to "zoe", it grants access to the Zoe framework. All contracts will need access to this.
* **feeMintAccess**: When this permission is set to "zoe", the contract will be able to create digital assets. Any contract that mints digital assets will need access to this.
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
* **feeMintAccess**: When this permission is set to "zoe", the contract will be able to create digital assets. Any contract that mints digital assets will need access to this.
* **feeMintAccess**: When this permission is set to "zoe", the contract will be able to create digital assets. Only contracts that mint privileged Agoric digital assets (not the unprivileged [`zcf.makeMint`](some link here)) will need access to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feeMintAccess's description has been changed to pretty much match your suggested text.

* **bankManager**: Grants access to bank-related functionality within ERTP and Zoe. Most contracts will need access.
* **board**: Grants write access to the board name service. This permission is somewhat specific to the PSM contract.
* **chainStorage**: Grants write access to the chain storage node, which is required when running `agd query` commands. Thus, most contracts will need access to this.
* **zoe**: When this permission is set to "zoe", it grants access to the Zoe framework. All contracts will need access to this.
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth mentioning above here that to "set" a permission, its value can be any truthy primitive. The value itself is not meaningful to the proposal (true, "zoe", "timer", "razzleflatz" and 249, are interchangeable and mean the same thing).

As a footnote, Agoric conventionally names its permission values for the identity of the home vat of the ocap corresponding to the permission. This can be useful for visualizing the produce/consume dependencies between actions.

Then, I'd change all the "permission is set to XXX" just to be "permission is set".

Suggested change
* **zoe**: When this permission is set to "zoe", it grants access to the Zoe framework. All contracts will need access to this.
* **zoe**: When this permission is set, it grants access to the Zoe framework. All contracts will need access to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An explanation was added to the top of the Permissions page saying that any string will set a permission. Similarly, all the permission descriptions were simplified per your suggestion above.

@dckc
Copy link
Member

dckc commented May 15, 2023

oops... I should have packaged up my comments into a reivew, I guess. But I thought I was just going to take a peek... then I saw 1 thing... then 1 more...

@Tyrosine22 Tyrosine22 requested review from michaelfig and dckc May 17, 2023 16:44
Write a json file with the permissions that the proposal will need. For example,
[gov-add-psm-permit.json](https://github.com/Agoric/agoric-sdk/blob/master/packages/inter-protocol/test/psm/gov-add-psm-permit.json)
is the proposal Agoric created for the PSM contract. Note that permissions are set by listing the
permission and setting it to any String. Thus,`"bankManager": true` and `"bankManager": false` are
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
permission and setting it to any String. Thus,`"bankManager": true` and `"bankManager": false` are
permission and setting it to any truthy value. Thus,`"bankManager": true` and `"bankManager": 'hello'` are

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the text per your suggestion.

* **zoe**: When this permission is set to "zoe", it grants access to the Zoe framework. All contracts will need access to this.
* **feeMintAccess**: When this permission is set to "zoe", the contract will be able to create digital assets. Any contract that mints digital assets will need access to this.
* **zoe**: When this permission is set, it grants access to the Zoe framework. All contracts will need access to this.
* **feeMintAccess**: When this permission is set, the contract will be able to create digital assets. Only contracts that mint privileged Agoric digital assets (i.e., not the unprivileged **[zcf.makeZDFMint()](/reference/zoe-api/zoe-contract-facet.md#zcf-makezcfmint-keyword-assetkind-displayinfo)**) will need access to this.
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
* **feeMintAccess**: When this permission is set, the contract will be able to create digital assets. Only contracts that mint privileged Agoric digital assets (i.e., not the unprivileged **[zcf.makeZDFMint()](/reference/zoe-api/zoe-contract-facet.md#zcf-makezcfmint-keyword-assetkind-displayinfo)**) will need access to this.
* **feeMintAccess**: When this permission is set, the contract will be able to create digital assets. Only contracts that mint privileged Agoric digital assets (i.e., not the unprivileged **[zcf.makeZCFMint()](/reference/zoe-api/zoe-contract-facet.md#zcf-makezcfmint-keyword-assetkind-displayinfo)**) will need access to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed,

@Tyrosine22
Copy link
Contributor Author

Can you please make requested changes and reply to questions.
How were the example shell commands tested? Can you add to the PR description a block of console output showing each example shell command being run and the successful output of them?

Here's the gist with the output: https://gist.github.com/Tyrosine22/6da2367ae8897736953c2f1286164eed

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.

It's getting there, but there are still a number of corrections that are important.

make scenario2-run-chain
```
5. Switch to another terminal and wait for the first block to be produced.
6. Navigate to `<agoric-sdk>/bin` and start the client.
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
6. Navigate to `<agoric-sdk>/bin` and start the client.
6. Navigate to `<agoric-sdk>/bin` and submit the governance proposal.

It doesn't start a client running.

Copy link
Member

Choose a reason for hiding this comment

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

And this isn't part of starting a local chain; it shouldn't be under than heading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the changes suggested above.

In addition, to address your second issue, (i.e., "it shouldn't be under that heading") I change the title of the topic to "Deploy a Governance Proposal to a Local Testnet" so that it properly reflects all the actions described in the topic.

Copy link
Member

Choose a reason for hiding this comment

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

I made the changes suggested above.

Did you push it yet? The most recent commit I see in this PR is on May 24 b22d2dd .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been pushed now.

You will need to write a proposal script that runs the contract, and possibly does additional things depending on your needs. (Usually these additional things will be dependent on a governance vote.) For example, [gov-add-psm.js](https://github.com/Agoric/agoric-sdk/blob/master/packages/inter-protocol/test/psm/gov-add-psm.js) is a proposal Agoric created for the PSM contract:

```jsx
/* global startPSM */
Copy link
Member

Choose a reason for hiding this comment

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

The startPSM global was provided in the pismo boot configuration but isn't provided in vaults.

The global environment for CoreEval scripts is another thing we should document:

https://github.com/Agoric/agoric-sdk/blob/bf164a05f425460ad357841b537b8dbc044357a2/packages/vats/src/core/chain-behaviors.js#L78-L83

where farExports comes from import * as farExports from '@endo/far'; and hence includes E and Far.
endowments comes from a few lines previous:

https://github.com/Agoric/agoric-sdk/blob/bf164a05f425460ad357841b537b8dbc044357a2/packages/vats/src/core/chain-behaviors.js#L40-L45

Documenting VatData is a whole other thing; maybe we can leave that out of scope for now.

And allPowers.modules comes from:

https://github.com/Agoric/agoric-sdk/blob/bf164a05f425460ad357841b537b8dbc044357a2/packages/vats/src/core/boot-chain.js#L17-L20

startPSM was part of allPowers.modules in the pismo bootstrap. But in vaults, it's not there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds like you're saying that startPSM isn't a part of CoreEval's global environment anymore. But gov-add-psm.js is checked-in code, it appears to assume that startPSM is part of its global environment, and the code works when I run it. So it seems like startPSM is still in global?

Copy link
Member

Choose a reason for hiding this comment

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

gov-add-psm.js is checked-in code

yes, but there are no automated tests for it

the code works when I run it.

That surprises me. There are no errors in the agd logs? And you can see a new PSM contract started with agd query vstorage children published.psm.IST?

Copy link
Member

Choose a reason for hiding this comment

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

@michaelfig , @Tyrosine22 says you were there when it worked for him. Any idea how the global reference to startPSM worked? I'm confused.

Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: just say "The script executes in an environment with globals such as E and Far provided." and leave enumeration of the whole list of globals to a later PR.

Copy link
Member

Choose a reason for hiding this comment

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

@michaelfig , @Tyrosine22 says you were there when it worked for him.

We verified that the vote passed and that something happened on the chain.

Any idea how the global reference to startPSM worked? I'm confused.

I don't know if the proposal contents were viable or completed successfully, only that they were initiated.

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 following text was added to the top blurb on proposal.md:

"The script executes in an environment with globals such as E and Far provided."

@@ -43,12 +44,20 @@ The Agoric SDK is supported on <a href="https://en.wikipedia.org/wiki/Linux">Lin
- To launch a terminal on Linux, use the **Terminal** application.
- To access WSL from Windows, visit the [WSL documentation](https://docs.microsoft.com/en-us/windows/wsl/).

## Install Node.js 16.19.1 or Higher
## Install jQuery
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
## Install jQuery
## Install jq

jq is just jq. jQuery is https://jquery.com/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. (And moved to another PR)

To create, start, and deploy an Agoric contract to a local Agoric Testnet, do the following:


1. Install *Go* if needed.
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
1. Install *Go* if needed.
1. Install *Go* if needed. For example:

Installing go with brew assumes a mac with brew installed. Our platform section says we support linux, MacOS, and WSL.

Building the go stuff is something we left out of the SDK 'till now. It merits its own section. In Getting Started. The idea was that it wasn't needed for smart contract development. With the smart wallet architecture, this is no longer really viable.

Copy link
Member

Choose a reason for hiding this comment

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

p.s. the https://docs.agoric.com/guides/dapps/starting-multiuser-dapps.html looks like it discusses building the go stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved the installation of Go to the Getting Started section in another PR.

Also, I changed the installation instructions so they're platform agnostic. (I hadn't realized that "brew" was Mac-specific.)

Comment on lines 14 to 20
```jsx
make scenario2-setup BASE_PORT=8000 NUM_SOLOS=0
```
4. Start the chain.

```jsx
make scenario2-run-chain
Copy link
Member

Choose a reason for hiding this comment

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

can we use agoric start local-chain instead? I'd rather not promote the scenario2 stuff in the cosmic-swingset Makefile to a product feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but let's do this in a separate PR; I was able to troubleshoot the "This project isn't in a folder created by agoric init" error, but I've run into a different error, and this one doesn't have a helpful error message. I'll ping you or mfig to work through 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.

All the "make scenario2" commands have been converted to various "agoric XXX" commands.

main/guides/coreeval/permissions.md Outdated Show resolved Hide resolved
main/guides/coreeval/permissions.md Outdated Show resolved Hide resolved
main/guides/coreeval/permissions.md Show resolved Hide resolved
main/guides/getting-started/README.md Outdated Show resolved Hide resolved
main/guides/getting-started/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,12 @@
# Injecting Code to Agoric Testnet

Agoric facilitates safely injecting code to the Agoric testnet, usually for the purpose of running a contract and modifying it based on the result of a governance vote. To do so, we submit a proposal using cosmos-sdk level API ([swingset.CoreEval](https://community.agoric.com/t/blder-dao-governance-using-arbitrary-code-injection-swingset-coreeval/99)). If the proposal passes its governance vote, its JavaScript code is run with ocaps extracted using the proposal's permit list, which the code can combine to perform privileged tasks.
Copy link
Member

Choose a reason for hiding this comment

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

the Agoric testnet? The term doesn't have a clear referent; it could be devnet or emerynet or ollinet... but what this PR seems to document is a local network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right... throughout this PR the reader is shown how to inject code into a local Testnet, but presumably they'd also be able to inject code into a remote Testnet.

Comment on lines 1 to 3
# Coding Permissions

Write a json file with the permissions that the proposal will need. For example,
Copy link
Member

Choose a reason for hiding this comment

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

Above it's called a "permit list". Here it's called "permissions". Any particular reason for the difference?

My preference would be...

Suggested change
# Coding Permissions
Write a json file with the permissions that the proposal will need. For example,
# Declaring Required Capabilities
Write a json file declaring the capabilities that the proposal will need. For example,

Copy link
Contributor Author

@Tyrosine22 Tyrosine22 Jun 13, 2023

Choose a reason for hiding this comment

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

No, there wasn't any real reason for the differences. They were just terms carried over from different sources. The terms should be standardized, but I wasn't sure which term best captures what's going on. I'm still not sure, actually.

I've changed the topic to largely conform to your "capabilities" language. The terms should be more standardized (permit list vs. permissions. vs capabilities), but (as I said above) I'm not 100% sure about which term I like best, and the topic reads OK as-is. So... it's good enough for now, I suppose.


Write a json file with the permissions that the proposal will need. For example,
[gov-add-psm-permit.json](https://github.com/Agoric/agoric-sdk/blob/master/packages/inter-protocol/test/psm/gov-add-psm-permit.json)
is the proposal Agoric created for the PSM contract. Note that permissions are set by listing the
Copy link
Member

Choose a reason for hiding this comment

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

"proposal" is yet another way to refer to it here. Suggestion:

Suggested change
is the proposal Agoric created for the PSM contract. Note that permissions are set by listing the
declares capabilities needed to start a new PSM contract. Note that capabilities are declared using their name as a property along with any truthy value.

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 text has been changed to the suggested passage.

```
:::

## Consume Section
Copy link
Member

Choose a reason for hiding this comment

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

There's more than one consume section: there's one at the top level and then one under issuer, one under brand etc.

Perhaps:

Suggested change
## Consume Section
## Top Level Section - Consume

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. The titles of the Top Level Consume & Top Level Produce sections have been changed to explicitly state that they're the top level.

main/guides/coreeval/permissions.md Outdated Show resolved Hide resolved
Comment on lines 95 to 96
Note that any **Brands** associated with digital assets that the contract mints
are not included in this section, unless they're also produced or consumed in some other capacity.
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why this "not included in this section" note is here. It seems wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was based on a misunderstanding I had of what the Brand & Issuer sections contained. I've deleted the note.

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 conveys the gist of what folks need to know, so I'm approving.

I would prefer that we finish 2 things before landing this, but they can probably be addressed later (or maybe even never) without much harm: The note about brands not being included doesn't seem right to me. Nor does the startPSM global.

@dckc dckc mentioned this pull request Jun 14, 2023
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.

still looks good

4. Switch to another terminal and wait for the first block to be produced.
5. Navigate to `<agoric-sdk>/bin` and submit the governance proposal.

5. Wait for the first block to be produced.
Copy link
Member

Choose a reason for hiding this comment

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

"How can I tell?" I can imagine our audience asking. I suggest providing an example log entry showing a block being produced.

(suggestion; not critical / important)

@michaelfig
Copy link
Member

@ivanlei, have your requested changes been addressed?

Copy link
Contributor

@ivanlei ivanlei left a comment

Choose a reason for hiding this comment

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

🚀

@Tyrosine22 Tyrosine22 merged commit 05c7693 into main Jun 20, 2023
@Tyrosine22 Tyrosine22 deleted the newcoreeval branch June 20, 2023 19:29
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.

4 participants