-
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
cleanup agoricNames publishing #9480
base: master
Are you sure you want to change the base?
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
|
||
// XXX will fail upon restart, but so would the makeStoredPublishKit this is replacing | ||
// Since we expect the bootstrap vat to be replaced instead of upgraded this should be | ||
// fine. See {@link ./README.md bootstrap documentation} for details. |
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.
Where is that link supposed to go? I don't see a README in this dir.
Possible alternative link:
84875d4
to
af9e1c1
Compare
@dckc PTAAL. To ensure Thinking in terms of least surprise, one could fairly expect a IOW, drop these last two commits. I'd still land the one removing the deprecated function. |
console.warn('no bridge so chainStorage will not write.'); | ||
chainStorageP.resolve(makeMockChainStorageRoot()); |
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! The undefined
case for chainStorage was only for the sim chain, I'm pretty sure.
console.warn
is exactly the way I think this should be treated.
A big part of me would like to tell the type system that this thing is always there so we don't have to keep supporting the sim chain in new code. That might be more involved, though.
cc @michaelfig @0xpatrickdev
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.
What would it take to remove sim-chain concerns from chain-behaviors entirely? I figure we could provide a fake bridge
device here:
agoric-sdk/packages/vats/src/core/chain-behaviors.js
Lines 319 to 328 in af9e1c1
if (!bridge) { | |
console.warn( | |
'Running without a bridge device; this is not an actual chain.', | |
); | |
bridgeManagerP.resolve(undefined); | |
provisionBridgeManager.resolve(undefined); | |
provisionWalletBridgeManager.resolve(undefined); | |
walletBridgeManager.resolve(undefined); | |
return; | |
} |
But I don't know what fidelity it needs or if anything downstream relies on detecting its absence.
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.
What would it take to remove sim-chain concerns from chain-behaviors entirely?
I can't think of any technical obstacles. @michaelfig , maybe you know?
Seems like product (@mitdralla, @sufyaankhan ) should be involved. @sufyaankhan led the charge to hide the sim chain from docs.agoric.com .
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.
What would it take to remove sim-chain concerns from chain-behaviors entirely?
I can't think of any technical obstacles. @michaelfig , maybe you know?
No technical obstacles. Earlier in our history, it was considered onerous to force dapp-developers to install Docker or compile agd themselves. Nowadays, we rely on Docker without batting an eye. The times have changed, and sim-chain is a relic we can safely jettison.
follow-up
Description
#9477 (comment)
Security Considerations
none
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
CI
Upgrade Considerations
This code ran only in tests