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

chore: cosmos interchain service cleanup #9810

Merged
merged 6 commits into from
Jul 31, 2024

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Jul 30, 2024

refs: #9114
closes: #9317

Description

  • Miscellaneous cleanup related to CosmosInterchainService (part of vat-orchestration), closing out todos for 1) unnecessary PowerStore abstraction and 2) vow compat for early synchronous returns
  • Optimizes ICQ Port Allocation by reusing a single port for all ICQ connections

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

js doc added where necessary

Testing Considerations

included tests for new behavior

Upgrade Considerations

these changes are intended to better prepare us for future upgrades

@0xpatrickdev 0xpatrickdev requested review from turadg and dckc July 30, 2024 22:52
Copy link

cloudflare-workers-and-pages bot commented Jul 30, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: d80a57c
Status: ✅  Deploy successful!
Preview URL: https://dfae7f14.agoric-sdk.pages.dev
Branch Preview URL: https://pc-cosmos-interchain-service.agoric-sdk.pages.dev

View logs

@@ -203,7 +186,7 @@ const prepareCosmosOrchestrationServiceKit = (
controllerConnectionId,
opts,
);
const portAllocator = getPower(this.state.powers, 'portAllocator');
const { portAllocator } = this.state;
Copy link
Member

Choose a reason for hiding this comment

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

yay for simpler. what happens if we need to provide new powers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely certain and figured it'd be easiest to get feedback in the PR.

I based reserved on what we did for localchain.js. This would indicate we can only add more powers here.

A more recent read of #7337 leads me to believe StateShape only matters if we specify opts.stateShape for the 5th arg? We can remove reserved and rely on ...powers in the initState function for new powers? If not, it'd be great to understand better.

Copy link
Member

Choose a reason for hiding this comment

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

only matters if we specify opts.stateShape

no; you can't add new properties, even without stateShape

Once the state is defined by the init function (3rd arg), properties cannot be added or removed.
-- https://docs.agoric.com/guides/zoe/contract-upgrade.html

reserved lets us put a new record there, or a store like powers was, should we need one.

Also, lacking that, there's the "side table" approach used to add non-vbank asset support to the smart wallet in #8071.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for clarifying @dckc . I added 9e16702 which spreads powers after reserved and defines reserved on OrchestrationPowers. This way, we can define extra powers via powers.reserved if needed.

@0xpatrickdev 0xpatrickdev force-pushed the pc/cosmos-interchain-service-cleanup branch 3 times, most recently from c51387c to 52f981b Compare July 31, 2024 03:25
@0xpatrickdev 0xpatrickdev force-pushed the pc/cosmos-interchain-service-cleanup branch from 52f981b to 7d3177c Compare July 31, 2024 14:41
@0xpatrickdev 0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Jul 31, 2024
- expose an optional argument for providing different ica channel parameters, such as version and ordering
- allow caller to specify optional version string
- update persitence logic to key off of ${controllerConnectionId}:${version}
- in the case of a potential early synchronous return, still return a vow
- a port can be reused to create multiple channels. requesting a new channel with the same port should result in a new channel id
- uses channelCount to increment channelID. keeps a map of channels per connection to determine counterpartyChannelID
- changes logic in provieICQConnection to lazily request a port on the first request, and reuse it in subsequent requests. since the channel
is ordered, timeouts and query errors will not affect subsequent queries and this can be considered safe
- closes #9317
@0xpatrickdev 0xpatrickdev force-pushed the pc/cosmos-interchain-service-cleanup branch from 7d3177c to d80a57c Compare July 31, 2024 14:57
@mergify mergify bot merged commit 8718c07 into master Jul 31, 2024
79 checks passed
@mergify mergify bot deleted the pc/cosmos-interchain-service-cleanup branch July 31, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optimize ICQ port allocation
3 participants