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

cleanup agoricNames publishing #9480

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

cleanup agoricNames publishing #9480

wants to merge 3 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Jun 10, 2024

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

@turadg turadg requested a review from dckc June 10, 2024 21:14
Copy link

cloudflare-workers-and-pages bot commented Jun 10, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: af9e1c1
Status: ✅  Deploy successful!
Preview URL: https://a7b2d03e.agoric-sdk.pages.dev
Branch Preview URL: https://ta-agoricnames-cleanup.agoric-sdk.pages.dev

View logs


// 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.
Copy link
Member

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:

@turadg turadg force-pushed the ta/agoricNames-cleanup branch from 84875d4 to af9e1c1 Compare June 12, 2024 17:14
@turadg
Copy link
Member Author

turadg commented Jun 12, 2024

@dckc PTAAL. To ensure chainStorage is always available, I had to mock it in chain-behaviors when there's no bridge. That's production code (or was, during bootstrap) so I'm not fully comfortable putting that in there. I could rename it from "mock" to "in-memory" or something like that.

Thinking in terms of least surprise, one could fairly expect a chainStorage capability to actually write to chain. So I think we're better off keeping it as null in the space when it won't run to chain and handling that case at the consumer sites.

IOW, drop these last two commits. I'd still land the one removing the deprecated function.

@turadg turadg requested review from dckc and gibson042 June 12, 2024 17:18
dckc

This comment was marked as outdated.

@dckc dckc dismissed their stale review June 12, 2024 17:50

I see PTAAL now

Comment on lines +359 to +360
console.warn('no bridge so chainStorage will not write.');
chainStorageP.resolve(makeMockChainStorageRoot());
Copy link
Member

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

Copy link
Member Author

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:

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.

Copy link
Member

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 .

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants