-
Notifications
You must be signed in to change notification settings - Fork 208
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
9063 chain lookup #9459
9063 chain lookup #9459
Conversation
no value in runtime
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.
This has some rough edges, but I support landing it.
I propose this is worth landing without IBC connection info
concur.
@@ -1,5 +1,10 @@ | |||
/** | |||
* @file Mocked Chain Info object until #8879 | |||
* @file Mocked Chain Info object |
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.
can we move this out of src/ yet?
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 gave it a try but will save it for a next PR because it affects the a3p test too.
packages/orchestration/src/facade.js
Outdated
// TODO zone is a lot of power to hand down. If it's only used for | ||
// `prepareCosmosOrchestrationAccount` we can do that in the parent scope |
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.
IIUC, making an async flow needs a durable zone.
I expect it can be a sub-zone and need not be the whole thing.
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.
ah yes. I'll remove this comment.
agoricNames: bootstrap.agoricNames, | ||
localchain: bootstrap.localchain, | ||
orchestrationService: bootstrap.orchestration, | ||
storageNode: bootstrap.storage.rootNode, |
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.
have we been passing the whole storage root to the contract? that seems weird.
(not a request for change)
agoricNames, | ||
localchain, | ||
orchestrationService: orchestration, | ||
storageNode: storage.rootNode, |
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.
hm. passing the whole storage root to a contract shouldn't be common
I'm not sure what to suggest
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 expect it'll change when we test writes to vstorage
refs: #9063 ## Description More progress on #9063 Following up on #9459 Next I think I'll set up a tool to grab the chain-registry info for the chains in the checklist and bring them into agoricNames In parallel, @dckc is populating agoricNames with the chain _connection_ info which can be appended to even after the chain (graph node) is defined. ### Security Considerations no changes ### Scaling Considerations no changes ### Documentation Considerations no need ### Testing Considerations CI, including A3P proposal update ### Upgrade Considerations no yet deployed
refs: #9063
Description
Progress on #9063, looking up primary chain info by chainID. Modeled after https://github.com/Agoric/agoric-sdk/tree/dc-unk-chain-misc
Doesn't yet handle IBC connection info (
hostConnectionId
,controllerConnectionId
). I propose this is worth landing without that but invite reviewers to weigh in with requests.Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations