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

chore(price-puhser) Use Hermes Client #2312

Merged
merged 12 commits into from
Feb 6, 2025
Merged

Conversation

aditya520
Copy link
Member

No description provided.

Copy link

vercel bot commented Jan 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
api-reference ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2025 3:43pm
component-library ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2025 3:43pm
entropy-debugger ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2025 3:43pm
insights ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2025 3:43pm
proposals ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2025 3:43pm
staking ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 6, 2025 3:43pm

@aditya520 aditya520 marked this pull request as ready for review January 31, 2025 16:23
@aditya520 aditya520 requested a review from ali-bahjati January 31, 2025 17:52
@aditya520 aditya520 marked this pull request as draft January 31, 2025 21:31
@aditya520 aditya520 marked this pull request as ready for review February 4, 2025 20:38
Copy link
Collaborator

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

This is a very good change. Thank you. Can you double check whether pusher keeps working or crashes upon network failures on Hermes? (look at my comment about it). A good way to test is to run it, turn off internet and wait a little while and turn it on again. A good behaviour is that it either crashes when you turn it off, or it doesn't crash but keeps working after you turn the internet on. anything else is a silent dead state.

Comment on lines +59 to +71
// Better to filter out invalid price items before creating the pyth listener
const { existingPriceItems, invalidPriceItems } =
await filterInvalidPriceItems(hermesClient, priceItems);

if (invalidPriceItems.length > 0) {
logger.error(
`Invalid price id submitted for: ${invalidPriceItems
.map(({ alias }) => alias)
.join(", ")}`
);
}

const priceItems = priceConfigs.map(({ id, alias }) => ({ id, alias }));
priceItems = existingPriceItems;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend you to do it in the PythPriceListener to avoid replicating the logic on each network

Copy link
Member Author

Choose a reason for hiding this comment

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

Either we filter pricefeeds at src/index.ts or at every chain level since we pass price ids to both chain listener and price listener.

@@ -67,16 +67,6 @@ export const logLevel = {
} as Options,
};

export const priceServiceConnectionLogLevel = {
"price-service-connection-log-level": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's remove it from readme as well

apps/price_pusher/src/pyth-price-listener.ts Show resolved Hide resolved
Comment on lines +74 to +77
eventSource.onerror = (error: Event) => {
console.error("Error receiving updates from Hermes:", error);
eventSource.close();
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think here you should either crash the service, or try to reconnect. doing otherwise will result in one dead component while the rest are working.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's better it crashes when the connection goes down. Just tested it.

package.json Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants