-
Notifications
You must be signed in to change notification settings - Fork 308
refactor(apps/price_pusher): crash on RPC failures #1730
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| version: objectResponse.data!.version, | ||
| }; | ||
| } else { | ||
| throw new Error("Failed to refresh object reference"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how often this may happen, but if it fails frequently and causes crashes, there is a higher chance that we lock all the gas price objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During my experiments I think I've resolved the issue of locked gases. It seems that it happens when we reuse a gas coin upon crash/restarts. I changed our deployment configuration to never allow to live instances at the same time and wait 10s before starting a new instance to make sure older ones are settled. No locking issues since this change.
I'll also run this code for sui for a day or so before merging to see how it works.
guibescos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debugged it a bit, found couple of bugs
87c3623 to
8de8d28
Compare
| logger: Logger | ||
| ): Promise<SuiObjectRef[]> { | ||
| const signerAddress = await signer.toSuiAddress(); | ||
| const signerAddress = signer.toSuiAddress(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by: linter
| pythPackageId, | ||
| pythStateId, | ||
| wormholePackageId, | ||
| wormholeStateId, | ||
| endpoint, | ||
| keypair, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by: linter
| this.logger.info({ signatures }, "updatePriceFeed successful"); | ||
| } catch (err: any) { | ||
| this.logger.error(err, "updatePriceFeed failed"); | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this return doesn't seem to do anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol it was there before and i removed it. added it back to minimize the changes.
This PR updates the price pusher code to cause a crash when the RPC has issues. The RPC issues can be either the target network or Hermes.
This also solves #1704.