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

Bring all ScalarBigXXXStore methods into consistency with the types defined in the stores package #7632

Closed
erights opened this issue May 6, 2023 · 5 comments · Fixed by #7668
Assignees
Labels
bug Something isn't working vaults_triage DO NOT USE

Comments

@erights
Copy link
Member

erights commented May 6, 2023

The MapStore and SetStore addAll methods are correctly typed and implemented by @agoric/store. But it lacks both explanation and tests.

The virtual and durable MapStores fail to implement addAll at all. The virtual and durable SetStores implementations of addAll are incomplete: they correctly recognize and process an Iterable argument. But they do not recognize a CopySet argument.

@erights erights added the bug Something isn't working label May 6, 2023
@erights erights changed the title MapStore,SetStore addAll method insufficiently implemented or tested. MapStore,SetStore addAll methods insufficiently implemented or tested. May 6, 2023
@erights
Copy link
Member Author

erights commented May 9, 2023

Also snapshot methods such as

snapshot: (keyPatt = undefined, valuePatt = undefined) =>
makeCopyMap(entries(keyPatt, valuePatt)),
are implemented only by heap stores, but not by virtual or durable ones. These are the dual of the addAll methods, so it makes sense that they were omitted together.

@FUDCo
Copy link
Contributor

FUDCo commented May 9, 2023

The virtual and durable SetStores implementations of addAll are incomplete: they correctly recognize and process an Iterable argument. But they do not recognize a CopySet argument.

I don't understand what this means. Are CopySets not iterable?

@FUDCo
Copy link
Contributor

FUDCo commented May 10, 2023

As I was working through how to apply the fixes for this, I discovered another error: it appears we have an internal-only operation on sets (addToSet) being added to the public API of all the collection classes. This seems to be the consequence of some unrelated cleanup. This was introduced in commit c81d367 (I poked around in the git log to see what idiot had done this and of course it was me). If @erights concurs I'd like to recast this issue as "Bring all ScalarBigXXXStore methods into consistency with the types defined in the stores package".

@erights
Copy link
Member Author

erights commented May 10, 2023

If @erights concurs

Concur!

@FUDCo FUDCo changed the title MapStore,SetStore addAll methods insufficiently implemented or tested. Bring all ScalarBigXXXStore methods into consistency with the types defined in the stores package May 10, 2023
@erights
Copy link
Member Author

erights commented May 10, 2023

I don't understand what this means. Are CopySets not iterable?

Please read endojs/endo#1583 ! Important (but very non urgent)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vaults_triage DO NOT USE
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants