-
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(orchestrate): membrane friendly wrapper for agoricNames #9551
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
* @param {Zone} zone | ||
* @param {{ agoricNames: Remote<NameHub>; vowTools: VowTools }} powers | ||
*/ | ||
export const makeAgNamesTools = ( |
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.
Reviewers might suggest a better name
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 we should avoid Ag
abbreviation in code. "Tools" isn't quite right because this is augmenting AgoricNames. We sometimes use Kit for that but more often that's for a multi-faceted object and this isn't.
AgoricNames
would be confused with the remote/vat and AgoricNameHub
could also be.
ResumableAgoricNames
conveys that it's wrapping the remove in vows, but that seems like an anti-pattern. We don't want to have to define a fetcher for each remote.
My suggestion is that instead we name it conspicuously not for production. E.g. AgoricNamesHack
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've renamed it to ResumableAgoricNamesHack
. Given it supports additional functionality - a vbank lookup by brand with local caching - we might consider renaming in the future. When we build getBrandInfo
and asAmount
this may be a good place to put them (or, we can move findBrandInVBank
there) cc @dckc.
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 is an interim approach until ...
What motivates an interim approach now? Why not either leave it as is or solve it completely?
const vbankAssetNameHubP = E(agoricNames).lookup('vbankAsset'); | ||
return watch( | ||
E(vbankAssetNameHubP).values(), | ||
Far('BrandsWatcher', { |
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.
Far
? Is that how watch
is supposed to be used? I don't see Far
used in watch.test.js. I see exos there.
I don't see any docs either way on the watch
function nor the Watcher
type.
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.
Refactored to a durable exo in 3cf3fd0
* @param {Zone} zone | ||
* @param {{ agoricNames: Remote<NameHub>; vowTools: VowTools }} powers | ||
*/ | ||
export const makeAgNamesTools = ( |
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 we should avoid Ag
abbreviation in code. "Tools" isn't quite right because this is augmenting AgoricNames. We sometimes use Kit for that but more often that's for a multi-faceted object and this isn't.
AgoricNames
would be confused with the remote/vat and AgoricNameHub
could also be.
ResumableAgoricNames
conveys that it's wrapping the remove in vows, but that seems like an anti-pattern. We don't want to have to define a fetcher for each remote.
My suggestion is that instead we name it conspicuously not for production. E.g. AgoricNamesHack
* interim approach until https://github.com/Agoric/agoric-sdk/issues/9541, | ||
* https://github.com/Agoric/agoric-sdk/pull/9322, or | ||
* https://github.com/Agoric/agoric-sdk/pull/9519 |
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.
thanks for referencing all these
* @returns {Vow<AssetInfo>} | ||
*/ | ||
findBrandInVBank(brand) { | ||
// XXX consider caching and refetching 1x if brand is not found, |
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.
why refetch? why only once?
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.
Likely more clear with this refactor: 3cf3fd0
@@ -77,7 +67,7 @@ export const start = async (zcf, privateArgs, baggage) => { | |||
const { chainName, destAddr } = offerArgs; | |||
const { give } = seat.getProposal(); | |||
const [[kw, amt]] = entries(give); | |||
const { denom } = await findBrandInVBank(amt.brand); | |||
const { denom } = await V.when(agNamesTools.findBrandInVBank(amt.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.
this should come from context, not the closure. Also then it doesn't need V.when()
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.
Moved to context. It seems like V.when()
is still needed until orchestrate
uses asyncFlow
(like on dt/af-spike
)
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.
close enough for now
const makeResumableAgoricNamesHackKit = zone.exoClassKit( | ||
'ResumableAgoricNamesHack', |
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 will fail if makeResumableAgoricNamesHack
is called more than once in the same zone.
Add that to the docstring? or just leave it implicit in the *Hack
name?
if (vbankAssetsByBrand.has(brand)) { | ||
return watch(vbankAssetsByBrand.get(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.
Can we skip the watch()
here and just return the payload directly? Wrapping it in a Vow
adds a round-trip between vats.
Does Vow<T>
include T
like ERef<T>
does?
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.
Add a TODO about cache invalidation?
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.
Added cache invalidation in aa53b01.
And yes, Vow<T>
is a thing.
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.
Can we skip the watch() here and just return the payload directly? Wrapping it in a Vow adds a round-trip between vats.
My understanding from this comment is that we always have to return a vow. Seeing in the ticket description methods that don't return immediately return Vows, which seems like it might conflict with this .
zone, | ||
makeRecorderKit, | ||
zcf, | ||
remotePowers.timerService, | ||
timerService, | ||
vowTools, | ||
chainHub, |
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.
Aside: do we really have a function with that many positional args? If it gets >3, they should be named.
b0de115
to
98f44fb
Compare
98f44fb
to
622ed61
Compare
This was refactored to use the newly added |
This was refactored to use the newly added asVow helper
Did you consider adding this to In #9491, my draft has a |
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'd rather we add this to chainHub
than make a whole file of tests that we'll probably discard. But I guess I'm ok with it for now.
refs: #9449
Description
Perform remote calls to agoricNames in membrane-friendly way. This is an interim approach until #9541, #9322, or #9519.