Integration test for attestations #413
Conversation
| @@ -167,7 +154,7 @@ | |||
| cfg_yaml += f""" | |||
| - group_name: longer_interval_sensitive_changes | |||
| conditions: | |||
| min_interval_secs: 10 | |||
| min_interval_secs: 3 | |||
There was a problem hiding this comment.
attest faster so we don't have to wait as long for the checking to work
third_party/pyth/pyth_utils.py
Outdated
| @@ -17,14 +20,15 @@ | |||
| # How long to sleep between mock Pyth price updates | |||
| PYTH_PUBLISHER_INTERVAL_SECS = float(os.environ.get("PYTH_PUBLISHER_INTERVAL_SECS", "5")) | |||
| PYTH_TEST_SYMBOL_COUNT = int(os.environ.get("PYTH_TEST_SYMBOL_COUNT", "11")) | |||
| PYTH_DYNAMIC_SYMBOL_COUNT = int(os.environ.get("PYTH_DYNAMIC_SYMBOL_COUNT", "2")) | |||
There was a problem hiding this comment.
both of the changes here are again to make the attestation checking complete more quickly
third_party/pyth/pyth_utils.py
Outdated
|
|
||
| # If above 0, adds a new test symbol periodically, waiting at least | ||
| # the given number of seconds in between | ||
| # | ||
| # NOTE: the new symbols are added in the HTTP endpoint used by the | ||
| # p2w-attest service in Tilt. You may need to wait to see p2w-attest | ||
| # pick up brand new symbols | ||
| PYTH_NEW_SYMBOL_INTERVAL_SECS = int(os.environ.get("PYTH_NEW_SYMBOL_INTERVAL_SECS", "120")) | ||
| PYTH_NEW_SYMBOL_INTERVAL_SECS = int(os.environ.get("PYTH_NEW_SYMBOL_INTERVAL_SECS", "60")) |
There was a problem hiding this comment.
Do you mind making it 30 seconds? To be even faster?
devnet/check-attestations.yaml
Outdated
| spec: | ||
| clusterIP: None | ||
| selector: | ||
| app: p2w-attest |
There was a problem hiding this comment.
I think this should also be "check-attestations"
|
|
||
| # Where to read the set of accounts from | ||
| PYTH_TEST_ACCOUNTS_HOST = "pyth" | ||
| PYTH_TEST_ACCOUNTS_PORT = 4242 |
There was a problem hiding this comment.
I think this should live in pyth_utils.py and be used by both this and pyth_publisher.py, but it's probably fine
third_party/pyth/pyth_utils.py
Outdated
| @@ -108,6 +111,20 @@ def sol_run_or_die(subcommand, args=[], **kwargs): | |||
| return run_or_die(["solana", subcommand] + args + ["--url", SOL_RPC_URL], **kwargs) | |||
|
|
|||
|
|
|||
| def get_json(host, path, port): | |||
There was a problem hiding this comment.
reorder to host, port, path?
| @@ -31,7 +31,7 @@ def do_GET(self): | |||
| self.wfile.flush() | |||
|
|
|||
| # Test publisher state that gets served via the HTTP endpoint. Note: the schema of this dict is extended here and there | |||
| HTTP_ENDPOINT_DATA = {"symbols": [], "mapping_address": None} | |||
| HTTP_ENDPOINT_DATA = {"symbols": [], "mapping_address": None, "all_symbols_added": False} | |||
There was a problem hiding this comment.
If I understand correctly, all_symbols_added makes the check-attestations dependency on pyth a special case. It is special, because an additional decision of the mock publisher logic permits check-attestations to go green.
I think that's reasonable, but can you note here what this is for?
| else: | ||
| logging.info("Price ids match but still waiting for more symbols to come online.") | ||
| else: | ||
| logging.info("Price ids do not match") |
There was a problem hiding this comment.
I think making if all_symbols_added the outermost test could be useful here. Inside we could do just a single config comparison and fail the whole script on mismatch. The publisher has a very well defined state when it's done adding symbols. This means that the expected attester state is also well defined and should be reached soon. The way this is handled now, it will be just a readiness timeout, which means a delay before any problems are reported. With this, failure of the python process would be visible as soon as it occurs.
There was a problem hiding this comment.
you can't fail here because there's a race condition between the attester and the HTTP endpoint which can cause this case to trigger (but the next loop iteration succeeds). Looping is more reliable.
| @@ -164,6 +164,9 @@ def add_symbol(num: int): | |||
| next_new_symbol_id += 1 | |||
| dynamically_added_symbols += 1 | |||
|
|
|||
| if dynamically_added_symbols >= PYTH_DYNAMIC_SYMBOL_COUNT: | |||
There was a problem hiding this comment.
I generally like the permissive comparison, but I think the > case would be a bug in this script, maybe we could yell about it and only be quiet for ==?
This PR adds a new job to tilt that checks that all symbols being published are also being attested. I tweaked some things on the publisher http endpoint to ensure that the check only succeeds once all of the dynamically-added symbols are generated. I also tweaked the configuration there to make tilt complete a little faster.