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.

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(smartWallet): defer deposits until purse available #6172

Merged
merged 4 commits into from
Sep 10, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Sep 9, 2022

supporting #6157

Description

Some brands aren't available when a payments to the deposit facet. In this case we now hold the payments in a queue and when the purse is available we deposit all of them.

The payouts handler now uses the deposit facet to get the same feature. It also attempts each payout separately instead of the previous behavior of blocking until all payments were available.

Security Considerations

Deposits are unpermissioned so queuing them opens an attack to grow the queue unbounded. The collections are durable which should mitigate RAM consumption. If there are more problems in practice the queue size can be capped. I considered doing that in this PR but considered a worse vulnerability is for an attacker to fill the queue of a wallet and prevent legitimate deposits.

Documentation Considerations

--

Testing Considerations

--

@turadg turadg requested review from dckc and dtribble September 9, 2022 20:11
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

get throws on our collections, no?

(reusing the standard name with different behavior was... an odd choice, IMO)

const amountPKeywordRecord = objectMap(paymentKeywordRecord, payment =>
E(purseForBrand(payment.getAllegedBrand())).deposit(payment),
const amountPKeywordRecord = objectMap(payouts, paymentRef =>
E.when(paymentRef, payment => depositFacet.receive(payment)),
Copy link
Member

Choose a reason for hiding this comment

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

👍 good idea to re-use the depositFacet

*/
async receive(payment) {
/** @type {State} */
const { brandPurses } = this.state;
const { brandPurses, paymentQueues: queues } = this.state;
const brand = await E(payment).getAllegedBrand();
const purse = brandPurses.get(brand);
Copy link
Member

Choose a reason for hiding this comment

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

This get will throw rather than returning undefined, no?

Copy link
Member Author

@turadg turadg Sep 9, 2022

Choose a reason for hiding this comment

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

yes good catch! I added a unit test for regression and it caught a missing harden too.

@turadg turadg requested a review from dckc September 9, 2022 22:24
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

thanks for the unit test

@turadg turadg changed the title smartWallet deposit queues feat(smartWallet): defer deposits until purse available Sep 10, 2022
@turadg turadg added the automerge:squash Automatically squash merge label Sep 10, 2022
@mergify mergify bot merged commit 1a1cc41 into master Sep 10, 2022
@mergify mergify bot deleted the ta/smartWallet-deposits branch September 10, 2022 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants