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.

Sign up for GitHub

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

Jump to bottom

fix: remove well-known-name limitation on reserved brands etc. #7604

Merged
merged 7 commits into from
May 5, 2023
Merged

fix: remove well-known-name limitation on reserved brands etc. #7604

merged 7 commits into from
May 5, 2023

Conversation

dckc
Copy link
Member

@dckc dckc commented May 4, 2023 •
edited
Loading

refs: #4548

Description

While refactoring the promise space to support a backing store, I it was straightforward to address a long-standing wart:

In the case of { brand: { consume: { abc } }, reserve abc
even though it's not among the statically defined reserved names.

Security / Scaling Considerations

none

Documentation Considerations

This reduces a burden on MN-2 core eval proposals, as discussed recently...
#6839 (comment)

Testing Considerations

happy-path unit tests for enhancements to PromiseSpace, NameHub APIs.

@dckc dckc requested a review from michaelfig May 4, 2023 06:14
@dckc dckc force-pushed the dc-space-reserve-any branch 2 times, most recently from 2f06b7e to 4e9bad6 Compare May 4, 2023 18:58
@dckc
Copy link
Member Author

dckc commented May 4, 2023

These are the symptoms reported in #7602, presumably fixed in #7612. So I suppose this is blocked by #7612.

not ok 10 - bootstrapTests › vaults-integration › propose change to auction governance param %ava-dur=5289ms
#   accept charter invitation
  ---
    name: AssertionError
    message: Rejected promise returned by test
    values:
      'Rejected promise returned by test. Reason:': |-
        Error {
          message: 'no invitation match (1 description and 0 instance)',

https://github.com/Agoric/agoric-sdk/actions/runs/4886307394/jobs/8721581067?pr=7604#step:5:3566

@dckc dckc marked this pull request as draft May 4, 2023 19:13
@dckc dckc marked this pull request as ready for review May 5, 2023 02:04
@dckc dckc requested a review from turadg May 5, 2023 02:05
dckc
dckc commented May 5, 2023
View reviewed changes
packages/vats/src/nameHub.js
@@ -85,7 +85,7 @@ export const makeNameHubKit = () => {

/** @type {import('./types.js').NameAdmin} */
const nameAdmin = Far('nameAdmin', {
provideChild(key) {
provideChild(key, reserved = []) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought i was going to add this later; I guess I got split-brain.

dckc
dckc commented May 5, 2023
View reviewed changes
packages/vats/src/core/types.js
* @typedef {{
* consume: Record<string, Promise<V>>,
* produce: Record<string, Producer<V>>,
* }} PromiseSpace
Copy link
Member Author

Choose a reason for hiding this comment

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

Odd; I thought I was using that in the hooks.

dckc
dckc commented May 5, 2023
View reviewed changes
packages/vats/src/core/promise-space.js
* ) | (typeof console.log)} [optsOrLog]
* @returns {PromiseSpace<V>}
* @returns {PromiseSpaceOf<T>}
Copy link
Member Author

Choose a reason for hiding this comment

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

ok. cool.

dckc
dckc commented May 5, 2023
View reviewed changes
packages/vats/src/types.js
@@ -36,18 +36,18 @@ export {};
* @property {(key: string) => Promise<void>} reserve Mark a key as reserved; will
* return a promise that is fulfilled when the key is updated (or rejected when
* deleted). If the key was already set it does nothing.
* @property {<T>( key: string, newValue: T, newAdmin?: unknown) =>
* @property {<T>( key: string, newValue: T, newAdmin?: NameAdmin) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

this is tricky... we can't really enforce it at runtime, but we sort of rely on it

Copy link
Member

Choose a reason for hiding this comment

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

for all our function signatures, the param types are meant to inform the caller. the function receiving the params can't trust anything except within a guarded Exo

Copy link
Member Author

Choose a reason for hiding this comment

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

right; we shouldn't rely on the caller giving the correct type at runtime; i wonder whether we do

@dckc dckc added the automerge:rebase Automatically rebase updates, then merge label May 5, 2023
@turadg turadg added automerge:no-update (expert!) Automatically merge without updates and removed automerge:rebase Automatically rebase updates, then merge labels May 5, 2023
@turadg
Copy link
Member

turadg commented May 5, 2023

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented May 5, 2023

refresh

✅ Pull request refreshed

@turadg
Copy link
Member

turadg commented May 5, 2023

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented May 5, 2023

requeue

☑️ This pull request is already queued

@turadg
Copy link
Member

turadg commented May 5, 2023

The chain deployment tests passed but Mergify is wedged so I'm going to try a fresh push with bypass.

@turadg turadg added the bypass:integration Prevent integration tests from running on PR label May 5, 2023
@turadg
Copy link
Member

turadg commented May 5, 2023

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented May 5, 2023

requeue

☑️ This pull request is already queued

@turadg turadg merged commit 35e5e30 into master May 5, 2023
@turadg turadg deleted the dc-space-reserve-any branch May 5, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turadg turadg turadg approved these changes

@michaelfig michaelfig Awaiting requested review from michaelfig

Assignees
No one assigned
Labels
automerge:no-update (expert!) Automatically merge without updates bypass:integration Prevent integration tests from running on PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dckc @turadg