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

Improvements to Orchestration Key-Concepts Page #1201

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

amessbee
Copy link
Contributor

@amessbee amessbee commented Sep 10, 2024

Here is a list of changes:

  1. Fixed errors in the structure + grammar
  2. Removed ICA figure from cosmos (theme looks out of place in our docs)
  3. Merged Key Concepts and API pages as they had, somewhat, fluffy content.

Orchestration Key Concept Page Rendered

Refs:

Copy link

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

Deploying documentation with  Cloudflare Pages  Cloudflare Pages

Latest commit: ecb07fb
Status: ✅  Deploy successful!
Preview URL: https://569ff001.documentation-7tp.pages.dev
Branch Preview URL: https://ms-update-orch-concepts.documentation-7tp.pages.dev

View logs

Copy link

github-actions bot commented Sep 10, 2024

Cloudflare deployment logs are available here

@amessbee amessbee marked this pull request as ready for review September 10, 2024 14:18
@amessbee amessbee changed the title Improvements to Orchestration Key-Concepts Page --WIP-- Improvements to Orchestration Key-Concepts Page Sep 10, 2024
@dckc dckc requested a review from mitdralla September 10, 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.

I defer to @mitdralla and @Jovonni

@mitdralla
Copy link
Contributor

  • The page (and docs) are really starting to come together. I have some feedback around the main navigation parents/children items. I think we could use more hierarchy to how the children links are organized - I think it is at the point where a category label could be helpful, and even further down the road a mega-menu type layout
image
  • For the side nav - "takeaway x:" seems unnecessary, I would just title and label it "Starting a Local Chain" and update the other items.
image
  • There is a lot of truncation on the right side nav, where the first words are more generic and the truncated part seems more important - consider cutting those words down so we can see the full title
image

Example: "Creating your Dapp from a Template" -> "Creating from a Template"
Example: "Starting the Dapp Smart Contract" -> "Starting the Smart Contract"

  • The page is long and could seem overwhelming for a Getting Started page. Consider adding in the copy up top "Get started in 5 steps" to help set some expectations.

or visit https://classic.yarnpkg.com/en/docs/install for more information -> or visit the official yarn package

  • I would guide them to the next logical location to hand hold them a bit more like "creating your first smart contract with our hello world example"
image

@amessbee
Copy link
Contributor Author

@mitdralla thanks for your comments. They are very helpful and I or @mujahidkay will create a separate issue based on your feedback. My current PR is about a (key concepts and APIs) page in our new orchestration section. If you can also provide feedback on that it would be great. Below is a direct link to the updated page that needs to be reviewed:

Orchestration Key Concepts

Sorry for the confusion - I should have been clearer when I requested feedback. Looking forward.

main/guides/orchestration/getting-started/key-concepts.md Outdated Show resolved Hide resolved
main/guides/orchestration/getting-started/key-concepts.md Outdated Show resolved Hide resolved
main/guides/orchestration/getting-started/key-concepts.md Outdated Show resolved Hide resolved
```

### ChainHub
## ChainHub
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this section (ChainHub) consistent with previous two sections with regard to listing different APIs separately. For example I think you can add two sub-headings for registerChain and registerConnection in this section.

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree, and take the opportunity to explain each. This is a good set of api functions though

Copy link
Contributor

Choose a reason for hiding this comment

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

@amessbee i'd say its good this is prob the only comment to address before merging.


### Orchestration Account
## Interchain Account (ICA)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please maket his section consistent by adding APIs subsections. Also currently it seems to be vague and marketing speak. For example, it is not clear to me at first what is ICA after all in the code example bellow.
Are omniAccount and localAccount ICA objects? If yes, then can you add getAddress, delegate, and transferSteps as API subsections in this ICA section?

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 was thinking of removing this subsection but kept it for now. I don't understand it well enough to be certain, but I believe we do not use the ICA in the code that is externally accessible to the developers. Rather we use the orchestration account that I already explain above.

@mitdralla
Copy link
Contributor

One more item which is minor - there is a missing space: "asAmountconverts a denom amount to an Amount with a brand."
image

@amessbee amessbee self-assigned this Sep 16, 2024
@amessbee amessbee force-pushed the ms/update-orch-concepts branch 2 times, most recently from d90ef38 to 9550bc3 Compare September 24, 2024 04:01
@amessbee amessbee force-pushed the ms/update-orch-concepts branch 2 times, most recently from ea29202 to d80469b Compare September 24, 2024 05:05
@turadg
Copy link
Member

turadg commented Sep 24, 2024

Removing myself. This is DevRel purview.

@turadg turadg removed their request for review September 24, 2024 14:21
@toliaqat toliaqat removed the request for review from rowgraus September 24, 2024 18:31
Copy link
Contributor

@toliaqat toliaqat left a comment

Choose a reason for hiding this comment

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

Looks very good. Thanks for updating this page. I am approving from the editorial side. From the technical side, @Jovonni can have a quick look. If there are any minor technical issues, I would request to make subsequent PRs after merging this.

main/guides/orchestration/getting-started/key-concepts.md Outdated Show resolved Hide resolved
### Registration APIs

- `registerChain` register a new chain with `chainHub`. The name will override a name in well known chain names. If a
durable zone was not provided, registration will not survive a reincarnation of the vat, and will have to be
Copy link
Member

Choose a reason for hiding this comment

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

I was able to follow the flow of these docs up until i read reincarnation of the vat especially since we haven't discussed it in orchestration docs before. What exactly is it? I think everything after the first sentence should be additional info presented in a tip section

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC "incarnation" represents before & after an upgrade.

After an upgrade you have a new incarnation. Maybe think of "version" when you see this.

Before an upgrade, the vat may be incarnation # 1, and then after an upgrade, the vat may be incarnation # 2

main/guides/orchestration/getting-started/key-concepts.md Outdated Show resolved Hide resolved
main/guides/orchestration/getting-started/key-concepts.md Outdated Show resolved Hide resolved
Copy link
Member

@mujahidkay mujahidkay left a comment

Choose a reason for hiding this comment

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

Great work! Please consider squashing the recent commits into an appropriate parent commit

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.

left a few comments, its coming together nicely


### Interchain Account (ICA)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why we are removing all education on interchain accounts? would like to understand this decision. If the team supports it then so be it, but I've found developers usually don't have an understanding on what an ICA is.

Although it can still be debated whether or not its useful, we have found it useful to explain it to them, and why its important. Other than that, orchestration accounts seem like magic.


A key advantage of ICAs is that they make accounts on other chains look like any other (remotable) object. When a contract creates an ICA, it has sole access to and control over the account but can delegate certain forms of access to its clients.
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason we are removing this?


Orchestration accounts are a key concept in the Agoric Orchestration API, represented by the [`OrchestrationAccountI`](https://agoric-sdk.pages.dev/interfaces/_agoric_orchestration.OrchestrationAccountI) interface. These accounts provide high-level operations for managing accounts on remote chains, allowing seamless interaction and management of interchain accounts. The orchestration accounts abstract the complexity of interchain interactions, providing a unified and simplified interface for developers.
```javascript
const [agoric, remoteChain] = await Promise.all([
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure its helpful, but I find myself being asked, "what does promise.all do?" more often that I expected when showing this 👀


- `getAddress` retrieves the address of the account on the remote chain.

```javascript
const address = await orchestrationAccount.getAddress()
```

**2. Balance Management**
### Balance Management
Copy link
Contributor

Choose a reason for hiding this comment

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

What is happening under the hood here, is IF we invoke getBalance on an remote account, this is actually an Interchain Query (ICQ). We can probably take the opportunity to briefly explain this here if this is the first time they see it.

Example from orch dapp contract:

const remoteChainBalance = await remoteAccount.getBalance('uosmo');

```

### ChainHub
## ChainHub
Copy link
Contributor

Choose a reason for hiding this comment

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

i agree, and take the opportunity to explain each. This is a good set of api functions though

@amessbee
Copy link
Contributor Author

@Jovonni thanks a lot for detailed review and comments. In essence, what we are doing here, everything that is not absolutely necessary for devs and posting it in a separate page: PR #1209.
We want this page to be magical and really abstract out all the details, conveying the message that if you need to orchestrate it is as simple as calling getChain, makeAccount, and then tansfer for you and Agoric will take care of everything under-the-hood, and BTW if you need to know what's going on under-the-hood, here is an other page with details.

WDYT?

@Jovonni Jovonni self-requested a review September 26, 2024 14:51
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 🚀 , this is the only thing to address:
https://github.com/Agoric/documentation/pull/1201/files#r1755891367

```

### ChainHub
## ChainHub
Copy link
Contributor

Choose a reason for hiding this comment

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

@amessbee i'd say its good this is prob the only comment to address before merging.

@amessbee
Copy link
Contributor Author

LGTM 🚀 , this is the only thing to address: https://github.com/Agoric/documentation/pull/1201/files#r1755891367

@Jovonni this comment by @toliaqat has already been addressed - if you look at the file chainHub section already has two subsections. Let me know if there are any further comments.

@amessbee amessbee force-pushed the ms/update-orch-concepts branch 3 times, most recently from 5ac83ac to 6914698 Compare September 30, 2024 16:20
@amessbee amessbee merged commit 20a3ecf into main Oct 2, 2024
6 checks passed
@amessbee amessbee deleted the ms/update-orch-concepts branch October 2, 2024 07:00
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.

7 participants