feat: make withdrawal volume metrics meter per day volumes #223
Conversation
montekki
requested review from
AnastasiiaVashchuk and
Deniallugo
as code owners
| /// | ||
| /// # Arguments | ||
| /// * `at_date_time`: Look for the block at this date-time | ||
| /// * `start_from_block`: Will perform search starting from this block, |
| /// * `start_from_block`: Will perform search starting from this block, | ||
| /// if `None` is specified then searches from block 1. | ||
| /// * `middleware`: The client to perform requests to RPC with. | ||
| pub async fn get_block_number_by_timestamp<M: Middleware>( |
There was a problem hiding this comment.
Why are you looking for the block in api? why not in the database?
There was a problem hiding this comment.
I don't save block timestamps to DB
There was a problem hiding this comment.
It'll simplify the code and you can forget about the old blocks
There was a problem hiding this comment.
This will not simplify the code, there is no info about each miniblock stored + no subscriptions to new l2 blocks, each l2 block is not processed and so on. The only place where they currently exist is l2_blocks table but that only contains effectively block ranges as we get them out of verify, commit and execute events on l1. So, every time i would get such a blockrange I will have to fetch all l2 block headers for thousands of blocks in the blockrange just to get their timestamps to populate this new row + this wouldn't be precise anyway.
OTOH the current solution makes only ~30 RPC requests on startup
| self.tokens.insert(w.event.token, (decimals, address)); | ||
| let mut guard = self.tokens.write().await; | ||
| guard.insert(w.event.token, (decimals, address)); | ||
| drop(guard); |
There was a problem hiding this comment.
why do you need to drop it manually? it will be dropped by it self just a few lines further
| Self { | ||
| let tokens = Arc::new(RwLock::new(token_decimals)); | ||
|
|
||
| tokio::spawn(reset_metrics_at_midnight(tokens.clone(), component_name)); |
There was a problem hiding this comment.
are you sure it's a good idea?
I'd rather nullify it once miniblock timestamp changes the day.
There was a problem hiding this comment.
so you may be re-syncing after a restart or something similar and you will receive many miniblocks that change the day in a short period of time, or whatver. you get the idea.
There was a problem hiding this comment.
I don't think it's a huge problem.
There was a problem hiding this comment.
two points though:
- it will may be zeroing the graph during the day
- again, as I've mentioned above, there currently is no subscription to l2 miniblocks, and introducing one would probably be the bigger evil.
|
I know that this is how we collect and represent this data currently (increment during the day and reset at 00:00UTC), but I think it's awkward - we could track the amount withdrawn as a simple counter and show We'd not need Even if (for any reason) we want to show the graph like we do now (staircase that resets to zero everyday at 00:00), we can achieve it with something like |
|
Thanks, @RomanBrodetski , Achieved by querying: 
|
This PR implement the per-day metering of withdrawal volumes, both requested and already finalized. Basically there are two things that have to be done: