Remove batching from event based decryption triggers#700
Conversation
konradkonrad
force-pushed
the
fix/remove_event_based_batching
branch
from
422c1f9 to
b4737dc
Compare
ActivationBlockNumber can not be negative, so no overflow risk here.
jannikluhn
left a comment
jannikluhn
left a comment
There was a problem hiding this comment.
Fix itself looks good, but some comments on the tests
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def query_db(sql: str, keyper_index: int = 0) -> str: |
There was a problem hiding this comment.
this function seems rather pointless and confusing, because all it does it flip arguments
| return addr | ||
|
|
||
|
|
||
| def get_helper_bytecode(data_dir: Path) -> str: |
There was a problem hiding this comment.
do we have a deploy script in the contracts repo that we can call instead, maybe even in the deploy task along the other contracts? Side effect would be that we don't redeploy it for every single run.
| time.sleep(poll_interval) | ||
|
|
||
|
|
||
| def assert_no_batching( |
There was a problem hiding this comment.
I think this test is a bit too detailed and single purpose for an end to end test. Instead, I would only test that the expected decryption key(s) are produced (similar to what we do in test-decryption
| @@ -0,0 +1,322 @@ | |||
| #!/usr/bin/env -S uv run --script | |||
There was a problem hiding this comment.
The test setup is meant for setting up a testing environment and repeatedly running manual tests, like requesting keys and checking that it behaves as expected. This looks more like a full automated e2e test. I think we should have those and that they should use the test setup, but they should not be part of it itself. The same applies to the already existing test-decryption (which is much smaller though).
To make it fit, I think this should be split up:
- the test contract should be deployed as part of the existing
deploytask - there should be a
submit-event-registrationtask (not sure which parameters are needed) - there should be something like a
trigger-eventtask to trigger the earlier registered event (not sure how it should be identified) - there should be a task to wait for the generated key, either with
wait-for-decryption-keyor a new task (again not sure which parameter is needed to identify the event)
This allows to run tests manually and repeatedly with more freedom, maybe even with different event parameters or trigger conditions.
| # DB exists with tables. Re-initialize if the stored address doesn't match the config. | ||
| config_addr = utils.keyper_address(config_path) | ||
| db_addr = db_ethereum_address(database_name) | ||
| if db_addr.lower() != config_addr.lower(): |
There was a problem hiding this comment.
this (and the other changes in this file) seem unnecessary, why would the address change? And if they change, silently redeploying doesn't seem like the right fix
| return None | ||
|
|
||
|
|
||
| def get_uups_proxy_address( |
There was a problem hiding this comment.
I think this function often returns None or continues instead of raising an error when there's actually a broken/unexpected input file
2 participants
This removes the "batching" when signing identities for event based decryption triggers. It also adds a
misebased integration test,mise run test-event-decryption, to ensure the correct behavior.Drive-by fixes:
postgresversion from.tool-versionsContext is issue #698 / #699
Fixes #699