hermes support

[anihamde]

Adding hermes support to Python SDK so that users can pull verified Pyth prices for consumption on target chains

[cctdaniel]

seems like the test test_hermes_return_price_feed_object failed because it attempted to use a network socket, which is blocked by pytest-socket -- I think you should mock the network request instead (unit tests should be deterministic, and independent of external factors like network availability), see example here

[cctdaniel]

the httpx.AsyncClient is instantiated but not explicitly closed, which could potentially lead to resource leaks, maybe use the client as a context manager to ensure it's properly closed e.g.

    async with self.client as client:
        data = (await client.get(url)).json()

[anihamde]

The issue is that with context manager usage, any one call will close the client which makes it impossible to open again. I think using asyncio.run manages the resource leakage by closing all tasks running within the event loop whenever it's shut down: https://github.com/python/cpython/blob/c0b0c2f2015fb27db4306109b2b3781eb2057c2b/Lib/asyncio/runners.py#L64-L79

[cctdaniel]

why does self.pending_feed_ids not require uniqueness?

[anihamde]

we always call list(set(self.feed_ids)), so the feed_ids will always be unique

[cctdaniel]

it's possible for self.pending_feed_ids to contain duplicates if the same feed IDs are added more than once before they are cleared, for e.g.

• add_feed_ids method is called with ['feed1', 'feed2']
• before any operation that clears self.pending_feed_ids is performed, add_feed_ids is called again with ['feed2', 'feed3']
• self.feed_ids will now contain ['feed1', 'feed2', 'feed3'] without duplicates because it's converted to a set and then back to a list
• however, self.pending_feed_ids will contain ['feed1', 'feed2', 'feed2', 'feed3'], where 'feed2' is duplicated.

[cctdaniel]

imo you can do something like this to make it cleaner

    # convert feed_ids to a set to remove any duplicates from the input
    new_feed_ids_set = set(feed_ids)
    
    # update self.feed_ids; convert to set for union operation, then back to list
    self.feed_ids = list(set(self.feed_ids).union(new_feed_ids_set))
    
    # update self.pending_feed_ids with only those IDs that are truly new
    self.pending_feed_ids = list(set(self.pending_feed_ids).union(new_feed_ids_set))

[cctdaniel]

another thing that we can possibly do here is to validate that the feed_ids passed in is in the format we expect
e.g.

def validate_feed_ids(self, feed_ids: list[str]):
    """
    Validates the format of feed IDs. Each ID should be a 64-character hexadecimal string,
    optionally prefixed with '0x'.
    """
    hex_pattern = re.compile(r'^0x[a-fA-F0-9]{64}$|^[a-fA-F0-9]{64}$')
    for feed_id in feed_ids:
        if not hex_pattern.match(feed_id):
            raise ValueError(f"Invalid feed ID format: {feed_id}")

and then call validate_feed_ids in the first line of add_feed_ids and write tests to ensure that invalid ids (e.g. 30 chars, invalid chars, etc) throw an error

[cctdaniel]

is there a specific reason to do this? otherwise I recommend to import it at top-level to improve readability and maintainability

[anihamde]

i think we did this so that we didn't import these when we were using non-websocket client--i think it was more relevant where we had the code before, I think here it's def better to move to top

[cctdaniel]

did you follow the v1 api schema? maybe good to implement for v2 instead since that is live already e.g. https://hermes.pyth.network/docs/#/rest/latest_price_updates
but happy to defer to @ali-bahjati on what the right action here is

[cctdaniel]

left some more comments but looks good to me, another comment I have is that if you run pytest --cov-report=html you can see that the current coverage is at 43%, would be good to increase it as much as possible

[cctdaniel]

another thing that we can possibly do here is to validate that the feed_ids passed in is in the format we expect
e.g.

def validate_feed_ids(self, feed_ids: list[str]):
    """
    Validates the format of feed IDs. Each ID should be a 64-character hexadecimal string,
    optionally prefixed with '0x'.
    """
    hex_pattern = re.compile(r'^0x[a-fA-F0-9]{64}$|^[a-fA-F0-9]{64}$')
    for feed_id in feed_ids:
        if not hex_pattern.match(feed_id):
            raise ValueError(f"Invalid feed ID format: {feed_id}")

and then call validate_feed_ids in the first line of add_feed_ids and write tests to ensure that invalid ids (e.g. 30 chars, invalid chars, etc) throw an error