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

docs: add VStorage documentation #1110

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ansabgillani
Copy link
Contributor

@ansabgillani ansabgillani commented May 31, 2024

Adding VStorage documentation
Attempt to improve the documentation in terms of VStorage from just reference to a centralized documentation for the feature.

Copy link

cloudflare-workers-and-pages bot commented May 31, 2024

Deploying documentation with  Cloudflare Pages  Cloudflare Pages

Latest commit: 93c3d02
Status: ✅  Deploy successful!
Preview URL: https://0dcdec0d.documentation-7tp.pages.dev
Branch Preview URL: https://awg-add-vstorage-documentati.documentation-7tp.pages.dev

View logs

Copy link

github-actions bot commented May 31, 2024

Cloudflare deployment logs are available here

main/reference/vstorage-ref.md Outdated Show resolved Hide resolved
main/reference/vstorage-ref.md Outdated Show resolved Hide resolved
main/reference/vstorage-ref.md Outdated Show resolved Hide resolved
main/reference/vstorage-ref.md Show resolved Hide resolved
main/reference/vstorage-ref.md Outdated Show resolved Hide resolved
main/reference/vstorage-ref.md Outdated Show resolved Hide resolved
Comment on lines 38 to 52
## VStorage with CLI
VStorage can be accessed via CLI using:

```sh
$ agd /[--node {url}] query vstorage {path}
```

For example:
```sh
$ agd query vstorage path published.agoricNames
children:
- brand
- installation
- instance
...
Copy link
Member

Choose a reason for hiding this comment

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

Ah. yes. this is the sort of material that I'd like to see in https://docs.agoric.com/guides/agoric-cli/agd-query-tx.html

The --node flag is discussed more thoroughly there. I don't see any reference to it here, so you might as well drop it here.

What's the / in /[--node {url}] by the way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/ was a typo. Fixing it in a commit.

The query goes to a local node at tcp://localhost:26657 by default. To use another node:
$ agd status --node https://devnet.rpc.agoric.net:443

Can you confirm this is same for query as well? I am not sure about it. There is no mention about --node parameter there. Maybe I can add information there and then refer it to that.

Copy link
Member

Choose a reason for hiding this comment

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

yes, --node applies to all agd query and agd tx sub-commands, IME.

The help text is oddly inconsistent, though: agd query --help | grep node is empty, as is agd query vstorage --help | grep node but:

$ agd query tx --help | grep node
      --node string        <host>:<port> to Tendermint RPC interface for this chain (default "tcp://localhost:26657")

@gibson042 Do you know of some reason why the help text is inconsistent? If not, I suppose we owe an issue?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, an issue would be good. I'm pretty sure this is because the Cobra library responsible for CLI option definition shows help for applicable options defined higher in the command hierarchy (so-called "persistent flags") only at an invocable command (which agd query tx is but e.g. agd query vstorage is not because it must be followed by "children" or "data" or "path" and agd query params is not because it must be followed by "subspace").

But there should be a way to do it, because "--chain-id" seems to work that way (e.g., it shows in both e.g. agd tx --help and [as a Global Flag] in agd tx gov --help).

main/reference/vstorage-ref.md Outdated Show resolved Hide resolved
main/reference/vstorage-ref.md Outdated Show resolved Hide resolved
main/reference/vstorage-ref.md Outdated Show resolved Hide resolved
@ansabgillani ansabgillani marked this pull request as ready for review June 7, 2024 12:12
@ansabgillani ansabgillani changed the title [Draft] docs: add VStorage documentation docs: add VStorage documentation Jun 7, 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.

re technical correctness: this change is mostly neutral. I don't think it introduces any new material. The "Common VStorage Examples" heading suggests that it's no longer exhaustive, but the content is just as exhaustive as it was, and the top-level heading still says "Reference".

I don't see any errors.

So the impact is mainly organizational / editorial. I defer to others on that.

## DApp UI and VStorage
As mentioned above, VStorage offers clients an access to data from contracts, including dApp UI. For detail tutorial on tools and libraries available for this connection is available [here](https://docs.agoric.com/guides/getting-started/ui-tutorial/querying-vstorage.html#querying-vstorage).

## Common VStorage Examples
Copy link
Member

Choose a reason for hiding this comment

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

The docs on this page were intended as not just common examples but an exhaustive reference of what's in vstorage, as part of #947.

If this PR is changing that, we would need to renegotiate with the folks involved in #947.

Comment on lines -25 to +139
## vstorage: published.\* keys
### vstorage: published.\* keys
Copy link
Member

Choose a reason for hiding this comment

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

Demoting these from h2 to h3 means they don't show up in the page TOC. Is that on purpose?

Comment on lines +201 to +203
`published.agoricNames.instance` contains _instances_ of important contracts.
The data at this key are the entries of the NameHub. Here we show the object comprised of those entries.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you meant to repeat this.

main/reference/vstorage-ref.md Show resolved Hide resolved
main/reference/vstorage-ref.md Show resolved Hide resolved
## VStorage Protobuf Service Definition
The protobuf definition of vstorage is [agoric.vstorage.Query](https://github.com/Agoric/agoric-sdk/blob/mainnet1B/golang/cosmos/proto/agoric/vstorage/query.proto#L11) package.
```
service Query {
Copy link
Contributor

Choose a reason for hiding this comment

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

good idea to put the proto here

})
)
```
The `chainStorage` node corresponds to the `published` key in the [vstorage hierarchy](https://docs.agoric.com/reference/vstorage-ref.html#vstorage-hierarchy). Using `E(chainStorage).makeChildNode(contractName)` gives the contract access to write to the `published.{contractName}` key and all keys under it.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe put a security warning here to ensure contracts aren't passed the top most storage node, because then they can write to any key under it. I made this mistake in the past.

could be something like

Note: Always ensure to only pass the storage node to your contract that you explicitly intend for it to use. This will avoid giving a contract too much write powers to write to unintended storage nodes.

What do you think @ansabgillani ?

Copy link
Contributor

@Jovonni Jovonni left a comment

Choose a reason for hiding this comment

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

lgtm after addressing @dckc comments on duplicate ### vstorage: well known contracts, and ensuring the h2 shows up in table of contents.

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.

5 participants