-
Notifications
You must be signed in to change notification settings - Fork 220
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: durable agoricNames, namesByAddress #7605
Conversation
b24025b
to
717a002
Compare
58f003e
to
43ec75d
Compare
206d58c
to
44fdc31
Compare
18ed498
to
4db2eb2
Compare
@@ -163,7 +163,7 @@ export const makeBootstrap = ( | |||
vatData.set(name, { root }); | |||
} | |||
} | |||
void rawBootstrap(vats, devices).catch(e => { | |||
return rawBootstrap(vats, devices).catch(e => { |
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.
😻
@@ -108,7 +108,7 @@ | |||
* @typedef {object} LiquidationStrategy | |||
* @property {() => KeywordKeywordRecord} keywordMapping | |||
* @property {(collateral: Amount, run: Amount) => Proposal} makeProposal | |||
* @property {(runDebt: Amount) => Promise<Invitation>} makeInvitation | |||
* @property {(debt: Amount) => Promise<Invitation>} makeInvitation |
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 pushed this stray commit by accident. It doesn't hurt but if you rebase please excise
packages/vats/src/nameHub.js
Outdated
keys: () => { | ||
return harden([...keyToRecord.keys()]); | ||
}, | ||
export const prepareNameHubKit = (zone = heapZone) => { |
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.
a prepare without a zone argument doesn't make sense to me. you only ever make a prepare
if you're eventually making it durable. I think we should require the zone.
Moreover to avoid dangling baggages I think the non-durable zones should warn when kinds are defined into them. It can only be an interim state during development. À propos,
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.
a prepare without a zone argument doesn't make sense to me.
agreed. changing.
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 think the non-durable zones should warn when kinds are defined into them.
good idea; not for this PR, OK?
packages/vats/src/nameHub.js
Outdated
const init1 = () => ({ | ||
/** @type {LegacyMap<string, PromiseKit<unknown>>} */ | ||
// Legacy because a promiseKit is not a passable | ||
keyToPK: makeLegacyMap('nameKey'), |
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.
Then use JS Map. LegacyMap
is not to be used for any new code. It's scheduled for deletion.
packages/vats/src/nameHub.js
Outdated
/** @type {any} */ | ||
const firstValue = keyToValue.has(first) | ||
? keyToValue.get(first) | ||
: keyToPK.get(first).promise; // or throw |
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.
JS Map doesn't throw on missing values. To get that back a clean option is our NonNullish
checked cast:
: keyToPK.get(first).promise; // or throw | |
: NonNullish(keyToPK.get(first)).promise; |
packages/vats/src/vat-agoricNames.js
Outdated
|
||
/** @param {import('@agoric/zone').Zone} zone */ | ||
const makeBrandStore = zone => { | ||
const brandStore = zone.mapStore('Brand'); |
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.
Took me a minute to see this wasn't making singletons. I'm still getting used to zones.
One thing that would help readability is moving const brandStore
below the kind definition. Kinds should come first. Anything that the kind def depends on should be obvious so it's clear what has to happen in the first crank of restart.
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.
right... refactored to just prepareNatBrand
, without a closure for the state.
packages/vats/src/vat-agoricNames.js
Outdated
); | ||
|
||
return { | ||
provide: (keyword, displayInfo) => |
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.
The plain provide
is for when what is provided is specified by the make
argument. In this case it's always making a brand (and that's not implied by the signature).
provide: (keyword, displayInfo) => | |
provideBrand: (keyword, displayInfo) => |
66c4cd7
to
9cb0057
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.
(after squashing the fixups and reverts)
@@ -67,7 +67,7 @@ const makeDefaultTestContext = async t => { | |||
test.before(async t => { | |||
t.context = await makeDefaultTestContext(t); | |||
}); | |||
test.after.always(t => t.context.shutdown()); | |||
test.after.always(t => t.context.shutdown?.()); |
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.
this seems like a bad idea to me. Every bootstrap test needs to shutdown the host storage.
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.
ok. gone.
- provideChild is async - update is _not_ async
home.myAddressNameAdmin is a NameAdmin with an extra getMyAddress() method. Here we use exo "attenuators" to - add getMyAddress() to a NameAdmin, making a MyAddressNameAdmin - override provideChild() in NamesByAddressAdmin, providing a kit whose nameAdmin is a MyAddressNameAdmin This obsoletes makeMyAddressNameAdmin and simplifies a previously delicate and fragile coordination between the smart wallet and the ag-solo wallet.
The datalock from pismo seems to be gone. This should facilitate diagnostics.
Co-authored-by: Turadg Aleahmad <turadg@agoric.com>
refs: #4548
Description
makeNameHubKit
with NameAdmin.provideChild()namesByAddress
kit to vat-provisioning.jsagoricNames
kit to its own vat, vat-agoricNames.jsFar
attenuators that were not worthwhileSecurity Considerations
Reduce responsibilities of the bootstrap vat; after moving these two name services, after bootstrap, the bootstrap vat should only be used for core eval.
Scaling Considerations
namesByAddress is now virtualized and durable and out of the bootstrap vat
Documentation Considerations
none?
Testing Considerations
normal-to-light unit testing