Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix!: in SmartWallet, if invitation is invalid, don't process offer #9240

Merged
merged 3 commits into from
Apr 17, 2024
Merged

fix!: in SmartWallet, if invitation is invalid, don't process offer #9240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
868a85c
fix: in SmartWallet, if invitation is invalid, don't process offer
Chris-Hibbert Apr 15, 2024
718743e
chore: lint happiness
Chris-Hibbert Apr 16, 2024
8e5a054
chore: comment improvements from review
Chris-Hibbert Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/smart-wallet/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"devDependencies": {
"@agoric/cosmic-proto": "^0.4.0",
"@agoric/swingset-vat": "^0.32.2",
"@agoric/zone": "^0.2.2",
"@endo/bundle-source": "^3.2.2",
"@endo/captp": "^4.1.1",
"@endo/init": "^1.1.1",
Expand Down
17 changes: 9 additions & 8 deletions packages/smart-wallet/src/smartWallet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,22 +972,23 @@ export const prepareSmartWallet = (baggage, shared) => {

const invitation = invitationFromSpec(offerSpec.invitationSpec);

const [paymentKeywordRecord, invitationAmount] = await Promise.all([
proposal?.give &&
deeplyFulfilledObject(
facets.payments.withdrawGive(proposal.give, offerSpec.id),
),
E(invitationIssuer).getAmountOf(invitation),
]);
const invitationAmount =
await E(invitationIssuer).getAmountOf(invitation);
Comment on lines +975 to +976
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 2. Begin executing offer
// No explicit signal to user that we reached here but if anything above
// failed they'd get an 'error' status update.

const withdrawnPayments =
proposal?.give &&
(await deeplyFulfilledObject(
facets.payments.withdrawGive(proposal.give, offerSpec.id),
));

seatRef = await E(zoe).offer(
invitation,
proposal,
paymentKeywordRecord,
withdrawnPayments,
offerSpec.offerArgs,
);
facets.helper.logWalletInfo(offerSpec.id, 'seated');
Expand Down
14 changes: 11 additions & 3 deletions packages/smart-wallet/test/test-invitation1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const mockBootstrapPowers = async (
log,
spaceNames = ['installation', 'instance', 'issuer', 'brand'],
) => {
/** @type {import('@agoric/vat-data').Baggage} */
const baggage = makeScalarMapStore('bootstrap');
Comment on lines +36 to 37
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anybody know why the type of baggage isn't inferred from makeScalarMapStore?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would depend on the work to infer types from shapes

adding a link to track motivations:

const zone = makeDurableZone(baggage);
const { produce, consume } = makePromiseSpace();
Expand Down Expand Up @@ -82,7 +83,9 @@ const makeTestContext = async t => {
const makeSpendableAsset = () => {
const tok1 = makeIssuerKit('Tok1');
const { issuer, brand } = bootKit.powers;
// @ts-expect-error new symbol
issuer.produce.Token1.resolve(tok1.issuer);
// @ts-expect-error new symbol
brand.produce.Token1.resolve(tok1.brand);
return tok1;
};
Expand Down Expand Up @@ -114,6 +117,7 @@ const makeTestContext = async t => {
/** @type {import('../src/smartWallet.js').BrandDescriptorRegistry} */
const registry = await makeRegistry();

/** @type {import('@agoric/vat-data').Baggage} */
const swBaggage = makeScalarMapStore('smart-wallet');

const secretWalletFactoryKey = Far('Key', {});
Expand Down Expand Up @@ -151,6 +155,7 @@ test.serial('handle failure to create invitation', async t => {
const invitationIssuer = powers.issuer.consume.Invitation;
const address = 'agoric1234';

// @ts-expect-error Test setup ensures that chainStorage resolution is not undefined. (see #8247)
const walletsStorage = E(chainStorage).makeChildNode('wallet');
const walletStorageNode = await E(walletsStorage).makeChildNode(address);

Expand All @@ -167,14 +172,14 @@ test.serial('handle failure to create invitation', async t => {
const amt = AmountMath.make(spendable.brand, 100n);
const pmt = await E(spendable.mint).mintPayment(amt);
await E(purse).deposit(pmt);
shared.thePurse = purse;
const slowWithdrawPurse = {
...purse,
withdraw: async a => {
await delay(100);
console.log('@@slow withdraw', a);
return E(purse).withdraw(a);
},
getCurrentAmount: () => purse.getCurrentAmount(),
};
return slowWithdrawPurse;
},
Expand All @@ -183,10 +188,13 @@ test.serial('handle failure to create invitation', async t => {
},
});

const theBank = makeBank();
shared.thePurse = await theBank.getPurse(spendable.brand);

const smartWallet = await makeSmartWallet({
address,
walletStorageNode,
bank: makeBank(),
bank: theBank,
invitationPurse,
});
shared.theWallet = smartWallet;
Expand Down Expand Up @@ -216,7 +224,7 @@ test.serial('handle failure to create invitation', async t => {
await delay(200);
});

test.serial.failing('funds should be back in the purse', async t => {
test.serial('funds should be back in the purse', async t => {
t.like(t.context.shared.thePurse.getCurrentAmount(), { value: 100n });
});

Expand Down