fix!: in SmartWallet, if invitation is invalid, don't process offer #9240
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
8e5a054
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://73c2c595.agoric-sdk.pages.dev |
| Branch Preview URL: | https://9239-badinvitation.agoric-sdk.pages.dev |
| const invitationAmount = | ||
| await E(invitationIssuer).getAmountOf(invitation); |
There was a problem hiding this comment.
Looks like we're reconsidering the design from...
Is there anybody we should check with? The record only shows you, me, and @turadg as party to that issue and its fix.
There was a problem hiding this comment.
If I was part of that discussion, I've forgotten it.
Thanks for the off-line discussion. I'm comfortable that given what we know now, this order is better.
| /** @type {import('@agoric/vat-data').Baggage} */ | ||
| const baggage = makeScalarMapStore('bootstrap'); |
There was a problem hiding this comment.
Does anybody know why the type of baggage isn't inferred from makeScalarMapStore?
There was a problem hiding this comment.
Baggage is MapStore<string, any> but makeScaleMapStore returns MapStore<unknown, unknown>.
It can take keyShape and valueShape in options, so it could eventually infer those two type parameters, but that would depend on the work to infer types from shapes.
There was a problem hiding this comment.
that would depend on the work to infer types from shapes
adding a link to track motivations:
| // @ts-expect-error It's a promise. | ||
| const walletsStorage = E(chainStorage).makeChildNode('wallet'); |
There was a problem hiding this comment.
| // @ts-expect-error It's a promise. | |
| const walletsStorage = E(chainStorage).makeChildNode('wallet'); | |
| // @ts-expect-error Test setup ensures that chainStorage resolution is not undefined. | |
| const walletsStorage = E(chainStorage).makeChildNode('wallet'); |
another approach is something like:
* @param {BootstrapPowers & NonNullChainStorage} powers
* @typedef {PromiseSpaceOf<{ chainStorage: StorageNode }>} NonNullChainStorageThere was a problem hiding this comment.
I think either of the above is good for this PR. You could also reference,
| const invitationAmount = | ||
| await E(invitationIssuer).getAmountOf(invitation); | ||
|
|
||
| const paymentKeywordRecord = |
There was a problem hiding this comment.
This is mutative, so no longer simply "prepare". It belongs under "2. Begin executing offer". Consider also naming the object like withdrawnPayments to it's more clear how it relates to the lifecycle of the request.
|
Why the |
Chris-Hibbert
force-pushed
the
9239-badInvitation
branch
from
06f557b
to
8e5a054
Compare
I think of ! as "incompatible behavior change", but if its really "breaking change", I'll drop it. |
…itation (#9236) refs: #9239 ## Description test handling payments in case of failure to create invitation; incorporate fix from ... - #9240 ### Security / Scaling / Documentation n/a ### Upgrade Considerations deployment involves upgrading walletFactory (see #9238) ### Testing Considerations swingsetTest (and thus walletDriver) doesn't provide access to balances, so bootstrap test was removed from this PR
…itation (#9236) refs: #9239 ## Description test handling payments in case of failure to create invitation; incorporate fix from ... - #9240 ### Security / Scaling / Documentation n/a ### Upgrade Considerations deployment involves upgrading walletFactory (see #9238) ### Testing Considerations swingsetTest (and thus walletDriver) doesn't provide access to balances, so bootstrap test was removed from this PR
…itation (#9236) refs: #9239 ## Description test handling payments in case of failure to create invitation; incorporate fix from ... - #9240 ### Security / Scaling / Documentation n/a ### Upgrade Considerations deployment involves upgrading walletFactory (see #9238) ### Testing Considerations swingsetTest (and thus walletDriver) doesn't provide access to balances, so bootstrap test was removed from this PR
refs: #9239
Description
When SmartWallet gets a request to exercise an invitation, it shouldn't create an offer if the invitation is broken.
Security Considerations
Creating an offer requires withdrawing funds, which have to be returned to the user. Better to check the invitation first and skip the rest.
Scaling Considerations
None
Documentation Considerations
None
Testing Considerations
We added a test and a fix.
Upgrade Considerations
We'd like to get this into the next available upgrade. There are no complex interactions with other updates.