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

reduce imports of @agoric/store #7949

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

reduce imports of @agoric/store #7949

wants to merge 5 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Jun 19, 2023

Description

Most of it the imports what @agoric/store was for is provided by Endo now. Also @agoric/vat-data.

This replaces some imports of @agoric/store to use what it wraps.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

This replaces some imports of @agoric/store to use what it wraps. Should we deprecate the package?

I don't understand the question. This would be grounds for deprecating those exports of @agoric/store that are merely re-exports from something lower level, like @endo/patterns, which I do agree would be good. Please do feel free to do this deprecation in this PR.

But there are still plenty of exports from @agoric/store that originate with @agoric/store. These should stay. And when @agoric/store becomes @endo/store, the imports from @agoric/store should eventually be changed to import from @endo/store instead.

Further down the road, I expect that most uses of exo-making and store-making APIs will migrate to zone-based APIs, at which point there will be little remaining direct use of @endo/exo, @*/store or @agoric/vat-data. But we're not there yet.

packages/SwingSet/src/kernel/state/kernelKeeper.js Outdated Show resolved Hide resolved
import { makeExo, keyEQ, makeScalarMapStore } from '@agoric/store';
import { keyEQ } from '@endo/patterns';
import { makeExo } from '@endo/exo';
import { makeScalarMapStore } from '@agoric/vat-data';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { makeScalarMapStore } from '@agoric/vat-data';
import { makeScalarMapStore } from '@agoric/store';

IIUC the purpose of this PR, I'd expect you to prefer to import makeScalarMapStore from @agoric/store, where it originates, rather than @agoric/vat-data which imports and re-exports it. This seems more consistent with your preference (which I share) to import, for example, makeExo from @endo/exo where it originates, rather than @agoric/store, which imports and re-exports it.

Likewise for other similar changes from @agoric/store to @agoric/vat-data below.

FWIW, I plan to migrate @agoric/store to become @endo/store, and to move most of @agoric/zone to @endo/zone. Longer term, I expect such manual use of makeExo and makeScalarMapStore to migrate to using the corresponding Zone APIs. Just FYI; doesn't affect this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I mistakenly had in mind that vat-data sourced the functionality from globalThis and thus didn't entail a package dependency on @agoric/store.

I'll revert the changes that preferred vat-data.

Comment on lines +10 to +14
import {
makeScalarMapStore,
prepareExo,
provideDurableMapStore,
} from '@agoric/vat-data';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import {
makeScalarMapStore,
prepareExo,
provideDurableMapStore,
} from '@agoric/vat-data';
import { makeScalarMapStore } from '@agoric/store';
import {
prepareExo,
provideDurableMapStore,
} from '@agoric/vat-data';

@@ -200,7 +208,7 @@ const modRelRel = (rel, step) =>
* @param {Timestamp | RelativeTime} right
* @param {bigint} v1
* @param {bigint} v2
* @returns {RankComparison}
* @returns {-1 | 0 | 1}
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

because the type wasn't resolving. since TS is structurally typed it is functionally no different.

Copy link
Member

@erights erights Jun 20, 2023

Choose a reason for hiding this comment

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

You understand vastly more than I about why the type might not be resolving, or how to get it to resolve, than I do. If this can be repaired at reasonable cost, I'd prefer that, since {RankComparison} communicates intent to future readers that {-1 | 0 | 1} does not. But if that's hard, the second is fine with a comment. (I'm much more concerned with human understandability than type-checking equivalence.)

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Nice and boring. The way it should be!

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

All my remaining unresolved conversations are minor, so LGTM.

Due to my tardiness (sorry!), this PR had lagged behind head. When you rebase, if there's anything you think I should look at, please let me know. Otherwise I'll let my LGTM stand.

Btw, I remain puzzled by "Should we deprecate the package?" in the PR comment. If it no longer applies, please remove before merging. Thanks.

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