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

feat(vaults)!: vstorage index node for managers #7150

Merged
merged 2 commits into from
May 2, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Mar 9, 2023

refs: #6111

Description

One important piece of #6111 to get out in the Vaults release: put the indexed vault managers under a managers key. Makes the managers list queriable by enumerating children of the node. (Without this to enumerate would require looking at all children of vaultFactory and filtering by string pattern.)

This breaks clients expecting managerN to be a child of vaultFactory. The one client using it,

Security Considerations

--

Scaling Considerations

--

Documentation Considerations

Updated the README

Testing Considerations

CI and updating dapps

@datadog-full-agoric
Copy link

Datadog Report

Branch report: 6111-rationalize-paths
Commit report: 934fc9a

agoric-sdk: 0 Failed, 0 New Flaky, 137 Passed, 2 Skipped, 2m 40.76s Wall Time

@turadg turadg force-pushed the 6111-rationalize-paths branch from 72bcd8d to b3c373c Compare April 29, 2023 16:45
@turadg turadg requested a review from samsiegart April 29, 2023 16:49
@turadg turadg marked this pull request as ready for review April 29, 2023 16:49
This breaks clients expecting managerN to be a child of vaultFactory
@turadg turadg force-pushed the 6111-rationalize-paths branch from b3c373c to 6e1f851 Compare May 1, 2023 23:51
@samsiegart
Copy link
Contributor

Oh yea this also breaks https://github.com/Agoric/dapp-inter/blob/main/src/service/vaults.ts#L256 and a few other lines. I'll make a PR and test locally

@samsiegart
Copy link
Contributor

Made Agoric/dapp-inter#107

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.

Tested manually on Agoric/dapp-inter#107

@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label May 2, 2023
@mergify mergify bot merged commit 609421b into master May 2, 2023
@mergify mergify bot deleted the 6111-rationalize-paths branch May 2, 2023 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants