Orch api factoring #9356

turadg · 2024-05-09T23:29:44Z

closes: #9207

Description

Refactoring the API types and perhaps implementation to satsisfy:

a types module specifying the public chain-agnostic interface for Orchestration
- it must not expose Cosmos SDK types
- it should not change
a types module for the IBC subclass of the public interface

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages · 2024-05-09T23:31:22Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`01e4ac2`
Status:	✅ Deploy successful!
Preview URL:	https://6b950dce.agoric-sdk.pages.dev
Branch Preview URL:	https://ta-orch-api.agoric-sdk.pages.dev

View logs

packages/orchestration/src/orchestration-api.d.ts

packages/cosmic-proto/package.json

0xpatrickdev

LGTM. Considering we paired on this, I suspect you may also want a ✅ from @dtribble before merging

turadg · 2024-05-13T16:59:35Z

Taking out of draft. There's one lint failure but it's on no-use-before-define and I'm making a separate PR to disable that rule,

allow use before define #9361

dckc

Let's talk more before going to .ts for vat code.

dckc · 2024-05-13T18:14:52Z

packages/orchestration/src/types.ts

+export type * from './cosmos-api.js';
+export type * from './exos/chainAccountKit.js';
+export type * from './exos/icqConnectionKit.js';
+export type * from './orchestration-api.js';


this looks circular. I can imaging that's not a ts error, but it seems unhelpful for thinking about things.

Is this a blocking request for the PR? I can untangle the circularity but I wouldn't want to do that until the API surface is agreed upon

Is this a blocking request for the PR?

no.

I'm trying to understand the proposal.

packages/orchestration/src/orchestration-api.ts

packages/orchestration/src/types.ts

dtribble · 2024-05-13T18:44:05Z

packages/orchestration/src/cosmos-api.ts

+ * Chain. Some operations accept any member of the equivalence class to
+ * effectively designate the corresponding token type on the target chain.
+ */
+export type Denom = string; // ibc/... or uist


This we would like to NOT be cosmos-specific.

I have heard that Gravity has an encoding of eth etc. denoms.

https://github.com/PeggyJV/gravity-bridge/blob/main/module/x/gravity/types/ethereum.go

I think they use gravity/ethcontractadress but that doesn't include the Chain ID. So we would want something more like eth/<chainId>/<contractAddress>. The key is that it fits into the Denom formats, so we can just use Denom everywhere.

I think we might have better luck with an object here. You are right to call out that for ethereum (and solana) the important identifiers are contractAddress and chainId - denoms do not come into play until someone decides to make a representation on an IBC chain.

Aside - other evm-interop chains do not follow the same baseDenom pattern as gravity . On axelar they look something like: dai-wei, dot-planck, uusdc, weth-wei. And on kava: erc20/tether/usdt. I think we'll want to preserve these values in the denom name space, and use additional keys (contractAddress, chainId) to facilitate building messages for non-IBC chains.

The general that we don't need to commit a particular, I don't think I would want an object for all of these, since we get a lot of them back that are well known in context. All those fit just fine in the model of string encoding. these are just arms that someone on their chain can decide to use. Everyone else uses indirect references to them.

packages/orchestration/src/cosmos-api.ts

packages/orchestration/src/types.ts

dtribble · 2024-05-13T19:13:30Z

packages/orchestration/src/types.ts

+  };
+  cosmos: { info: CosmosChainInfo; methods: {} };
+  agoric: {
+    info: Omit<CosmosChainInfo, 'ibcConnectionInfo'>;


Omit is magic. What's this mean?

How is it magic?

https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys

Seems more clear to construct types additively rather than with subtraction. So what is the name of the type that doesn't include those operations?

fair. Agoric is one special case among Cosmos chains so I don't think it needs its own type alias, but I'll change it to Pick instead of Omit

use info so it looks like a localhost connection. Eventually it may be replaced with that implementation.

packages/orchestration/src/types.ts

dtribble · 2024-05-13T19:15:21Z

packages/orchestration/src/types.ts

+export type DenomArg = Brand | Denom;
+
+/** Amounts can be provided as pure data using denoms or as native Amounts */
+export type AmountArg = CoinAmount | Amount;


Nit: Order of "agoric type | low level type" should match between DenomArg and AmountArg.

For usability, I expect the low-level type should be first.

based on mis-read / misunderstanding

dtribble

Turadg and I walked through comments and agreed on the results. I consider it auto-approved after he makes those changes.

refactor(types): extract ethereum-api

refactor(types): Chain, OrchestrationAccount

to fix build order sensitivity

no ticket ## Description Started as documentation for #9356 (comment) , then I carried out the new best practice so it would be consistent. That was a bit of a rabbit hole. It's in a better place, though I don't see it reflected obviously in the type coverage report. ### Security Considerations n/a, types ### Scaling Considerations n/a, types ### Documentation Considerations This documents the new beset practice ### Testing Considerations CI ### Upgrade Considerations n/a, types

dtribble reviewed May 10, 2024

View reviewed changes

packages/orchestration/src/orchestration-api.d.ts Outdated Show resolved Hide resolved

packages/orchestration/src/orchestration-api.d.ts Outdated Show resolved Hide resolved

packages/orchestration/src/orchestration-api.d.ts Outdated Show resolved Hide resolved

dckc mentioned this pull request May 10, 2024

feat: redelegate, undelegate on StakingAccountKit #9331

Merged

4 tasks

turadg force-pushed the ta/orch-api branch from c0b0380 to 89c7096 Compare May 10, 2024 21:37

turadg requested a review from 0xpatrickdev May 10, 2024 21:37

0xpatrickdev reviewed May 10, 2024

View reviewed changes

packages/cosmic-proto/package.json Outdated Show resolved Hide resolved

0xpatrickdev approved these changes May 10, 2024

View reviewed changes

turadg force-pushed the ta/orch-api branch 2 times, most recently from dfbc592 to 7d51e2f Compare May 10, 2024 22:57

turadg requested a review from dtribble May 10, 2024 22:57

turadg marked this pull request as ready for review May 13, 2024 16:59

dckc previously requested changes May 13, 2024

View reviewed changes

dtribble reviewed May 13, 2024

View reviewed changes

turadg mentioned this pull request May 14, 2024

clean up types modules #9364

Merged

turadg requested review from 0xpatrickdev, dtribble and dckc May 14, 2024 14:24

turadg force-pushed the ta/orch-api branch from d27356a to 769ef76 Compare May 14, 2024 18:43

turadg removed request for dckc and 0xpatrickdev May 14, 2024 22:27

dtribble approved these changes May 14, 2024

View reviewed changes

turadg added 4 commits May 14, 2024 17:07

build: .ts

c413d10

chore(types): checkJs

ea4352d

chore(types): drop ambients

90b3002

refactor(types): extract ethereum-api

8c9a62b

refactor(types): extract ethereum-api

turadg force-pushed the ta/orch-api branch from 769ef76 to ccccd0e Compare May 15, 2024 00:09

turadg added the automerge:rebase Automatically rebase updates, then merge label May 15, 2024

refactor(types): extract cosmos-api

a211e88

turadg added 8 commits May 15, 2024 09:06

feat(types): orchestrate() generic

8fd74d0

refactor(types): Chain, OrchestrationAccount

refactor(types): extract chain-info module

fef4f2d

chore(types): asAmount returns Nat

b2546d4

refactor(types): rollup module

a51db10

docs: clarify Denom

29baf90

refactor: ChainAmount → DenomAmount

95ee056

build: consistent postpack

df2fed7

chore(types): more explicit imports

d1a35d8

to fix build order sensitivity

turadg force-pushed the ta/orch-api branch 2 times, most recently from c2fedbc to 952045a Compare May 15, 2024 16:39

ci: pack packages in PR tests

01e4ac2

turadg force-pushed the ta/orch-api branch from 952045a to 01e4ac2 Compare May 15, 2024 16:52

mergify bot merged commit 2dc53d7 into master May 15, 2024
63 checks passed

mergify bot deleted the ta/orch-api branch May 15, 2024 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Orch api factoring #9356

Orch api factoring #9356

turadg commented May 9, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 9, 2024 •

edited

Loading

0xpatrickdev left a comment

turadg commented May 13, 2024 •

edited

Loading

dckc left a comment

dckc May 13, 2024

turadg May 13, 2024

dckc May 13, 2024

dtribble May 13, 2024

dtribble May 13, 2024

dtribble May 13, 2024

0xpatrickdev May 14, 2024

dtribble May 14, 2024

dtribble May 13, 2024

turadg May 13, 2024

dtribble May 14, 2024

turadg May 14, 2024

dtribble May 14, 2024

dtribble May 13, 2024

dtribble May 13, 2024

dtribble left a comment

Orch api factoring #9356

Orch api factoring #9356

Conversation

turadg commented May 9, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented May 9, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

0xpatrickdev left a comment

Choose a reason for hiding this comment

turadg commented May 13, 2024 • edited Loading

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtribble left a comment

Choose a reason for hiding this comment

turadg commented May 9, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 9, 2024 •

edited

Loading

turadg commented May 13, 2024 •

edited

Loading