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: robustify allManagersDo #9833

Merged
merged 3 commits into from
Aug 6, 2024
Merged

feat: robustify allManagersDo #9833

merged 3 commits into from
Aug 6, 2024

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #9706

Description

make allManagersDo robust to errors thrown by one vaultManager

Security Considerations

Prevent issues in one vaultManager from blocking progress in others. This would affect liquidation and locking oracle prices for upcoming liquidation auctions.

Scaling Considerations

Doesn't add work.

Documentation Considerations

Not user visible

Testing Considerations

added a tiny unit test.

Upgrade Considerations

added robustness shouldn't impact upgrade.

@Chris-Hibbert Chris-Hibbert added code-style defensive correctness patterns; readability thru consistency Vaults VaultFactor (née Treasury) labels Aug 2, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Aug 2, 2024
Copy link

cloudflare-workers-and-pages bot commented Aug 2, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 193b443
Status: ✅  Deploy successful!
Preview URL: https://ee3c6ec8.agoric-sdk.pages.dev
Branch Preview URL: https://9706-allmgrsdo.agoric-sdk.pages.dev

View logs

@Chris-Hibbert Chris-Hibbert requested a review from dckc August 3, 2024 00:09
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.

maybe add a tiny bit of docs?

@@ -82,6 +82,18 @@ const trace = makeTracer('VD', true);

const shortfallInvitationKey = 'shortfallInvitation';

// exported for testing
export const makeAllManagersDo = (collateralManagers, vaultManagers) => {
Copy link
Member

Choose a reason for hiding this comment

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

swallowing errors like this seems unusual enough to merit a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. I also added a red flashing light 🚨

Copy link
Member

Choose a reason for hiding this comment

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

Should this be yellow? we seem to be recovering from it.

But I guess it does indicate a bug - a "this should never happen" situation, not just some client mis-behaving or something. So ok, yes, red.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be yellow? we seem to be recovering from it.

We recover from the loop, but we don't do anything about the individual vaultManager that died while setting up liquidations or locking prices. I think that's mostly okay because it will get another opportunity the next time around, but if one manager keeps failing, it should get attention.

@Chris-Hibbert
Copy link
Contributor Author

maybe add a tiny bit of docs?

I have no objection, but I can't think of an appropriate place to write more. What did you have in mind? Is this just the comment on the function that you requested?

@dckc
Copy link
Member

dckc commented Aug 6, 2024

... What did you have in mind? Is this just the comment on the function that you requested?

yes.

@Chris-Hibbert Chris-Hibbert added the automerge:squash Automatically squash merge label Aug 6, 2024
@mergify mergify bot merged commit e1baf4e into master Aug 6, 2024
79 checks passed
@mergify mergify bot deleted the 9706-allMgrsDo branch August 6, 2024 17:04
rabi-siddique pushed a commit that referenced this pull request Aug 7, 2024
closes: #9706 

## Description

make `allManagersDo` robust to errors thrown by one vaultManager

### Security Considerations

Prevent issues in one vaultManager from blocking progress in others. This would affect liquidation and locking oracle prices for upcoming liquidation auctions.

### Scaling Considerations

Doesn't add work.

### Documentation Considerations

Not user visible

### Testing Considerations

added a tiny unit test.

### Upgrade Considerations

added robustness shouldn't impact upgrade.
kriskowal pushed a commit that referenced this pull request Aug 27, 2024
closes: #9706 

## Description

make `allManagersDo` robust to errors thrown by one vaultManager

### Security Considerations

Prevent issues in one vaultManager from blocking progress in others. This would affect liquidation and locking oracle prices for upcoming liquidation auctions.

### Scaling Considerations

Doesn't add work.

### Documentation Considerations

Not user visible

### Testing Considerations

added a tiny unit test.

### Upgrade Considerations

added robustness shouldn't impact upgrade.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge code-style defensive correctness patterns; readability thru consistency Vaults VaultFactor (née Treasury)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible improvement in vaultDirector's allManagersDo()
2 participants