-
Notifications
You must be signed in to change notification settings - Fork 207
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: postalSvc contract forwards payments using namesByAddress (prototype) #8547
Conversation
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.
Seems fine. Some requests for mostly minor cleanups or clarification. The for loop issue seems substantive.
// XXX partial failure? | ||
for await (const pmtP of values(payouts)) { | ||
const pmt = await pmtP; | ||
await sendTo(recipient, pmt); |
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.
earlier failures will block later attempts. A Promise.all()
would try them in parallel, so that they can succeed independently.
* | ||
* @param {ZoeService} zoeMethods | ||
*/ | ||
const wrapZoe = zoeMethods => { |
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 is this helpful?
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.
to override installBundleID
p.s. thanks for taking a look.
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 don't get it. It looks like it passes installBundleID()
through just like it does for all the other methods it supports. In a test, why is this better than just passing zoeMethods
through? Did I misread something?
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 real zoe is an exo, so you can't just go { ...zoe, installBundleID: myMockInstallBundle }
. (I should probably document what happens if you try.)
wrapZoe
takes the zoe exo and gives you back a plain record/object, where you can override methods using spread expressions like that:
const zoe = Far('ZoeService', {
...wrapZoe(zoeService),
installBundleID: install1BundleID,
});
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.
Just so you're aware of bindAllMethods
/** |
As it says there, bindAllMethods
was only a transitional measure until replaced by code this PR's wrapZoe
, so no change suggested. Just FYI.
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; I was only vaguely aware of it. But I'm leery to use stuff from @agoric/internal
, because it's a private package, and I was thinking this contract should probably be outside the SDK.
4271d27
to
a246b54
Compare
To avoid conflicts with long standing release branches and their special CI treatment, use another branch name Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Disable journaling and full synchronous pragma modes during import
It wasn't working with `any` functions
- flesh out bob / author
Agoric/agoric-sdk#8547 2023-11-17 19:10 0040bea4a feat: postalSvc contract forwards payments using namesByAddress
Agoric/agoric-sdk#8547 2023-11-17 19:10 0040bea4a feat: postalSvc contract forwards payments using namesByAddress
Agoric/agoric-sdk#8547 2023-11-17 19:10 0040bea4a feat: postalSvc contract forwards payments using namesByAddress
work moved to agoric-labs/ag-power-tools#2 |
Description
This contract is a reusable component for sending payments using
namesByAddress
to look up adepositFacet
.A contract such as GiMiX or swaparoo only needs this instance in its terms.
Or an offer can go straight to this contract. The script here starts the contract with the Zoe Fee and Invitation issuers.
It's something of a work-around for the regression in the wallet where this feature wasn't shipped in production.
I'm not sure it should be merged, let alone shipped in production. Maybe it should be moved to an example app.
cc @michaelfig @samsiegart @dtribble
Security Considerations
TODO:
Testing Considerations
currently: