-
Notifications
You must be signed in to change notification settings - Fork 206
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: make PSM bootstrap support multiple brands #6146
Conversation
282d169
to
dd4887d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like good progress toward the target.
const newPsmFacets = { | ||
psm: E(governorFacets.creatorFacet).getInstance(), | ||
psmGovernor: governorFacets.instance, | ||
psmCreatorFacet: E(governorFacets.creatorFacet).getCreatorFacet(), | ||
psmAdminFacet: E(governorFacets.creatorFacet).getAdminFacet(), | ||
psmGovernorCreatorFacet: governorFacets.creatorFacet, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we're going to regret storing these promises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're probably right.
/** @typedef {import('./econ-behaviors.js').EconomyBootstrapSpace} EconomyBootstrapSpace */ | ||
|
||
/** @param {BootstrapSpace & EconomyBootstrapSpace & { devices: { vatAdmin: any }, vatPowers: { D: DProxy }, }} powers */ | ||
// /** @param {BootstrapSpace & { devices: { vatAdmin: any }, vatPowers: { D: DProxy }, }} powers */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to keep this commented-out line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope.
installation: { | ||
produce: { contractGovernor, committee, binaryVoteCounter, psm }, | ||
}, | ||
}) => { | ||
psmFacets.resolve(makeScalarMapStore()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps leave a comment for our future selves about what this is doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
refs: #6142
refs: #6139
Description
Make PSM bootstrap support multiple brands
This helps with #6142, but might be merged into that or #6139.
Security Considerations
The PSM facets are available throughout PSM. That seems acceptable.
Documentation Considerations
None
Testing Considerations
Existing tests continue to pass. Since some are using PSM bootstrap, we can tell that it successfully stored one PSM and retrieved it. I presume two wouldn't be harder, but haven't written tests.