Skip to content

6969 fix vaults error recovery #7020

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
Merged
merged 6 commits into from
Feb 21, 2023
Merged

6969 fix vaults error recovery #7020

merged 6 commits into from
Feb 21, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Feb 16, 2023 •
edited
Loading

closes: #6969

Description

When a vault failed to init, the seat wouldn't be exited. This resulted in the funds hanging on the seat and not being returned to the caller. The fix was to move the exit() to finally.

This then sometimes errored with stagings due to #7024 . The solution is to stop using stages, so this does that for vaultFactory as part of #7025. It also does it for stakeFactory because they have a coupling through a shared a dependency on chargeInterest.

Along the way I tried making a smart wallet integration test to detect the #6969 but it's only with vaults and we can't test that integration until #6880. I left the test in anyway.

This also has a CLI feature that was helpful while debugging,

And a drive-by for a different state problem,

Security Considerations

Prevents a theoretical exploit,

Scaling Considerations

--

Documentation Considerations

--

Testing Considerations

Using start-local-chain made vaults that used to error and lose funds. Then made the fix and verified the funds are restored after error.

@turadg turadg requested a review from dckc February 16, 2023 23:12
@dckc
Copy link
Member

dckc commented Feb 16, 2023

This motivates giving clients the ability to exit #6906.

dckc
dckc reviewed Feb 16, 2023
View reviewed changes
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.

notes so far...

packages/agoric-cli/src/commands/wallet.js
} catch (e) {
console.error('CAUGHT HERE', e);
}
execFileSync('agd', [`query`, 'bank', 'balances', opts.from], {
Copy link
Member

Choose a reason for hiding this comment

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

indexing under ambient authority #2160

(not to mention sync io)

packages/inter-protocol/src/vaultFactory/vaultManager.js Outdated
@@ -863,6 +864,8 @@ export const prepareVaultManagerKit = (
vaultId,
'never stored during initVaultKit failure',
);
} finally {
Copy link
Member

Choose a reason for hiding this comment

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

this seems to be a different try/catch than the one that used to contain seat.exit()

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! My eye was fooled into doing the wrong curly matching for some reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good eyes! Thankfully the tests caught this too

@turadg turadg marked this pull request as draft February 17, 2023 00:39
@turadg turadg requested a review from dckc February 17, 2023 19:14
@turadg turadg marked this pull request as ready for review February 17, 2023 19:21
@turadg turadg requested a review from Chris-Hibbert February 17, 2023 19:37
Chris-Hibbert
Chris-Hibbert suggested changes Feb 17, 2023
View reviewed changes
packages/inter-protocol/src/stakeFactory/stakeFactoryKit.js
Comment on lines +305 to +307
[clientSeat, vaultSeat, { [KW.Attestation]: giveColl }],
[vaultSeat, clientSeat, { [KW.Attestation]: wantColl }],
[clientSeat, vaultSeat, { [KW.Debt]: give }],
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be

Suggested change
[clientSeat, vaultSeat, { [KW.Attestation]: giveColl }],
[vaultSeat, clientSeat, { [KW.Attestation]: wantColl }],
[clientSeat, vaultSeat, { [KW.Debt]: give }],
[clientSeat, vaultSeat, { [KW.Attestation]: giveColl, [KW.Debt]: give }],
[vaultSeat, clientSeat, { [KW.Attestation]: wantColl }],

Do you prefer the existing version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that and the line wrapping made it less readable. I think this style make it more clear what's going on, coming from the stageDelta style.

packages/inter-protocol/test/vaultFactory/vault-contract-wrapper.js Outdated
/** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} */
const transfers = [
...nonMintTransfers,
[stage, vaultFactorySeat, { Minted: fee }],
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename stage, and drop the stage.clear() on line 97. It's no longer being used for staging, and people familiar with stagings will be confused by this naming.

Are there other calls to .clear() in vaults or staking that can also be dropped?

packages/inter-protocol/test/vaultFactory/vault-contract-wrapper.js Outdated
// Make best efforts to burn the newly minted tokens, for hygiene.
// That only relies on the internal mint, so it cannot fail without
// there being much larger problems. There's no risk of tokens being
// stolen here because the staging for them was already cleared.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no staging remaining, right? What is this referring to?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just copied over. But that means I should revert the change here and go update vaultDirector and stakeFactory to not talk about staging.

packages/inter-protocol/test/vaultFactory/vault-contract-wrapper.js Outdated
Comment on lines 110 to 113
Object.values(stage.getCurrentAllocation()).every(a =>
AmountMath.isEmpty(a),
),
X`Stage should be empty of Minted`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop this?

@turadg turadg requested a review from Chris-Hibbert February 18, 2023 00:53
Chris-Hibbert
Chris-Hibbert approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

LGTM. Very clean. Thanks.

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Feb 21, 2023
@erights erights mentioned this pull request Feb 21, 2023
Merged
erights
erights reviewed Feb 21, 2023
View reviewed changes
packages/zoe/src/contractSupport/atomicTransfer.js Outdated
Comment on lines 7 to 10
export const TransferPartShape = M.arrayOf(
M.or(SeatShape, AmountKeywordRecordShape),
);
Copy link
Member

Choose a reason for hiding this comment

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

Seems surprisingly loose. Trying a tighter pattern at #7040 , but do not yet know what CI has to say about it.

Have not yet looked at the TransferPart type, but clearly they should be co-maintained

erights
erights requested changes Feb 21, 2023
View reviewed changes
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

#7040 green. TransferPart type already consistent with suggested TransferPartShape

@erights
Copy link
Member

erights commented Feb 21, 2023

Only requested changes because I saw this PR was already on the merge train. Now that it's knocked off, I'll approve (with my suggestion), but remove the automerge label first.

@erights erights removed the automerge:rebase Automatically rebase updates, then merge label Feb 21, 2023
erights
erights approved these changes Feb 21, 2023
View reviewed changes
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Not fully reviewed, but approving because I have no objections, and approving is the only way I know to cancel a "Request changes". Because this approval has an outstanding suggestion, I also removed the automerge label first.

@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Feb 21, 2023
@mergify mergify bot merged commit a10b4e8 into master Feb 21, 2023
@mergify mergify bot deleted the 6969-fix-recovery branch February 21, 2023 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erights erights erights approved these changes

@Chris-Hibbert Chris-Hibbert Chris-Hibbert approved these changes

@dckc dckc Awaiting requested review from dckc

Assignees
No one assigned
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

incomplete recovery from hitting vaults minting limit
4 participants
@turadg @dckc @erights @Chris-Hibbert