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

fix(vaults): durable invitationMakers #7296

Merged
merged 1 commit into from
Mar 31, 2023
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Mar 31, 2023

Description

The Far object for invitationMakers in the vault offer result cannot be stored durably. This fixes that by making it a durable facet. Combined with #7294 the vault offer result can be stored durably.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

CI

@turadg turadg requested review from mhofman and dckc March 31, 2023 16:51
@dckc
Copy link
Member

dckc commented Mar 31, 2023

I can see how CI would make sure this doesn't cause regressions, but normally a fix comes with a test that shows what was broken. Is that not cost-effective?

Perhaps the stress tests coming from #7219 are what show that this fixes something?

@turadg
Copy link
Member Author

turadg commented Mar 31, 2023

normally a fix comes with a test that shows what was broken. Is that not cost-effective?

Good question! I don't see it as cost effective to do in CI because we'd have to observe the Zoe seat. That would require one of:

  • instrumentation on the ephemeralOfferResultStore size
  • emitting events for failure to store durably
  • a way for CI to grep the console output

The test-vaults-performance that runs out of CI could do by observing the heap growth of many retained offer results. I think that's possible using process.memoryUsage() but we'd need to fix #7297 and have the BYOD invoker that @warner is working on.

I agree we should try to have a test before closing that so I'll add that to the ticket.

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.

good idea

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Mar 31, 2023
@turadg turadg force-pushed the ta/durable-invitationMakers branch from 3feb942 to aec82e8 Compare March 31, 2023 21:24
@mergify mergify bot merged commit 397b0d0 into master Mar 31, 2023
@mergify mergify bot deleted the ta/durable-invitationMakers branch March 31, 2023 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants