-
Notifications
You must be signed in to change notification settings - Fork 214
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
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
@@ -203,7 +186,7 @@ const prepareCosmosOrchestrationServiceKit = ( | |||
controllerConnectionId, | |||
opts, | |||
); | |||
const portAllocator = getPower(this.state.powers, 'portAllocator'); | |||
const { portAllocator } = this.state; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c51387c
to
52f981b
Compare
52f981b
to
7d3177c
Compare
- 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
7d3177c
to
d80a57c
Compare
refs: #9114
closes: #9317
Description
CosmosInterchainService
(part ofvat-orchestration
), closing out todos for 1) unnecessary PowerStore abstraction and 2) vow compat for early synchronous returnsSecurity 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