Driver settle queue tests #3133
Conversation
squadgazzz
force-pushed
the
driver-settle-queue-tests
branch
from
287ccc1
to
5b27c13
Compare
squadgazzz
force-pushed
the
driver-settle-queue-tests
branch
from
a5ab68a
to
7bcd92f
Compare
| impl Default for Arguments { | ||
| fn default() -> Self { | ||
| Arguments::parse_from([""]) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this is needed 🤔
There was a problem hiding this comment.
In case only deriving the Default trait, values from clap are not used.
There was a problem hiding this comment.
but in that case, we could make these Arguments as optional
There was a problem hiding this comment.
In that case, the caller side would need to define default values, no? Didn't get the idea here.
There was a problem hiding this comment.
What I meant is something like this: #2924
There was a problem hiding this comment.
That didn't really help. Making this config optional would mean that the caller side will be responsible for the values that need to be used by default. While with the current approach, only one struct controls the default values.
There was a problem hiding this comment.
Using the ethrpc configurations seems weird to make this test work.
Did you consider using anvil's helper methods instead?
You could first disable auto mining and manually clear the mempool to make sure that transactions actually do not get mined so that the submission futures don't return.
| .len(); | ||
| assert_eq!(unique_solutions_count, solution_ids.len()); | ||
|
|
||
| // `collect_vec` is required to execute futures in the same order. |
There was a problem hiding this comment.
I'm pretty sure this is not true. Can you explain why collecting into a vec should make a difference here?
There was a problem hiding this comment.
Without collecting, the order of the results will be unpredictable.
| test.solve(), | ||
| test.solve(), | ||
| test.solve(), | ||
| test.solve(), |
There was a problem hiding this comment.
Would be good to make /solve return errors when the queue is already full. Otherwise the solver could continue winning auctions and later get slashed because it can no longer submit them.
There was a problem hiding this comment.
Great idea, I will address it in the original PR then.
squadgazzz
force-pushed
the
driver-settle-queue-tests
branch
2 times, most recently
from
de75f95
to
2efbcfe
Compare
Oh, that is cool, didn't know about that. Will try to use this instead. |
Description
An attempt to properly test #3129. The idea is to send multiple
/settlerequests to the same driver to check that some will be discarded due to the settlement queue limit. To achieve that, I found the easiest way to introduce some delay in the settlement process on the driver's side by using the existing ethrpc batch delay, which is used to wait for a certain amount of time before sending the call to accumulate a batch.It turned out, that this config was hardcoded, so it was required to enable it in the arguments first to use properly in tests.
Changes
Important
ethrpc_max_batch_size = 20must be added to the Driver's infra config since the default value is100, which is currently used by other components. And only Driver uses20. https://github.com/cowprotocol/infrastructure/pull/2301How to test
This is the test.