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

synchronous makeVstorageKit #10670

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

synchronous makeVstorageKit #10670

wants to merge 7 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Dec 11, 2024

incidental

refs: agoric-labs/vstorage-viewer#2

Description

In the course of working on agoric-labs/vstorage-viewer#3 it was hard to work with an async makeVstorageKit. It was async because construction of the kit required fetching a bunch of data from vstorage to populate agoricNames.

That concern is a higher level than vstorage so this pulls it out. That allows makeVstorageKit to be synchronous.

I added a test of makeAgoricNames both before and after.

Eventually I think something should make it easy to make an AgoricNamesKit that wraps VstorageKit the way VstorageKit wraps Vstorage. In hindsight I might have saved a bunch of refactoring if I'd done that in this PR. But it's done now and this way we can be more intentional about AgoricNamesKit.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

not documented but this will make it less complex

Testing Considerations

new test

While refactoring, tsc really let me down by not detecting destuctured properies that were no longer on the source object. I suppose our settings aren't strict enough. I enabled noImplicitAny to detect those (by then filtering for makeVstorageKit)

Upgrade Considerations

n/a

@turadg turadg requested a review from a team as a code owner December 11, 2024 02:09
@turadg turadg added the force:integration Force integration tests to run on PR label Dec 11, 2024
Copy link

cloudflare-workers-and-pages bot commented Dec 11, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2e6f750
Status: ✅  Deploy successful!
Preview URL: https://21f3547a.agoric-sdk.pages.dev
Branch Preview URL: https://ta-sync-vstoragekit.agoric-sdk.pages.dev

View logs

Copy link
Contributor

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

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

The CI failure looks like it might be related to the changes, will TAL after that's passing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants