Bridge batch report #2166
Bridge batch report #2166
Conversation
| @@ -10,14 +10,14 @@ use crate::{ | |||
| }; | |||
| use actix::prelude::*; | |||
| use std::{collections::HashSet, sync::Arc, time::Duration}; | |||
| use web3::contract::tokens::Tokenize; | |||
| @@ -68,12 +68,20 @@ impl DrReporter { | |||
| } | |||
| } | |||
|
|
|||
| /// Report the result of this data request id to ethereum | |||
| /// Report the result of this data requests to ethereum | |||
There was a problem hiding this comment.
| /// Report the result of this data requests to ethereum | |
| /// Report the results of this data requests to ethereum |
| // Data Request Bytes | ||
| //pub dr_bytes: Bytes, |
There was a problem hiding this comment.
if we are not using, should we remove it?
| { | ||
| let dr_database_addr = DrDatabase::from_registry(); | ||
| dr_database_addr.send(set_dr_info_bridge_msg).await.ok(); | ||
| // The request is already resolved, remove it from list |
There was a problem hiding this comment.
Should we modify this message?
| @@ -1,3 +1,4 @@ | |||
| use crate::actors::dr_reporter::Report; | |||
| @@ -1,3 +1,4 @@ | |||
| use crate::actors::dr_reporter::Report; | |||
| @@ -68,12 +68,20 @@ impl DrReporter { | |||
| } | |||
| } | |||
|
|
|||
| /// Report the result of this data request id to ethereum | |||
| /// Report the result of this data requests to ethereum | |||
There was a problem hiding this comment.
Even better:
| /// Report the result of this data requests to ethereum | |
| /// Report the results of these data requests to Ethereum |
| "Request [{}] is already being resolved, ignoring DrReporterMsg", | ||
| msg.dr_id | ||
| ); | ||
| // Remove all reports that have already been reported, but the transaction is pending |
There was a problem hiding this comment.
| // Remove all reports that have already been reported, but the transaction is pending | |
| // Remove all reports that have already been reported, but whose reporting transaction is still pending |
| // Remove all reports that have already been reported, but the transaction is pending | ||
| msg.reports.retain(|report| { | ||
| if self.pending_report_result.contains(&report.dr_id) { | ||
| // Timeout not elapsed, abort |
There was a problem hiding this comment.
| // Timeout not elapsed, abort | |
| // Timeout is not over yet, no action is needed |
| { | ||
| let dr_database_addr = DrDatabase::from_registry(); | ||
| dr_database_addr.send(set_dr_info_bridge_msg).await.ok(); | ||
| // The request is already resolved, remove it from list |
There was a problem hiding this comment.
| // The request is already resolved, remove it from list | |
| // The request was already resolved, we can remove it from the list |
| log::error!("reportResultBatch{:?}: {:?}", params_str, e); | ||
| } | ||
| Err(_e) => { | ||
| // Timeout elapsed |
There was a problem hiding this comment.
| // Timeout elapsed | |
| // Timeout is over |
| } | ||
| Err(_e) => { | ||
| // Timeout elapsed | ||
| log::warn!("reportResultBatch{:?}: timeout elapsed", params_str); |
There was a problem hiding this comment.
| log::warn!("reportResultBatch{:?}: timeout elapsed", params_str); | |
| log::warn!("reportResultBatch{:?}: timeout is over", params_str); |
| // The gas price of the report transaction should be the maximum gas price of any | ||
| // request |
There was a problem hiding this comment.
| // The gas price of the report transaction should be the maximum gas price of any | |
| // request | |
| // The gas price of the report transaction should equal the maximum gas price paid | |
| // by any of the requests being solved here |
tmpolaczyk
force-pushed
the
bridge-batch-report
branch
2 times, most recently
from
35cf787
to
70f6988
Compare
Co-authored-by: Luis Rubio <[email protected]>
Co-authored-by: Luis Rubio <[email protected]>
By ensuring that only one function exists with any given name
tmpolaczyk
force-pushed
the
bridge-batch-report
branch
from
07d11e0
to
96a58dc
Compare
tmpolaczyk
force-pushed
the
bridge-batch-report
branch
from
e3feeee
to
425e5f4
Compare
| @@ -10,14 +10,14 @@ use crate::{ | |||
| }; | |||
| use actix::prelude::*; | |||
| use std::{collections::HashSet, sync::Arc, time::Duration}; | |||
| use web3::ethabi::Token; | |||
| }; | ||
|
|
||
| let block_number = block.block_header.beacon.checkpoint; | ||
| // TODO: get constants from somewhere instead of hardcoding them here? |
There was a problem hiding this comment.
There is a jsonrpc method that returns the consensus constants
| // transactions are created one epoch earlier. | ||
| // TODO: first block with commits is hard to obtain, we are simply using the | ||
| // block that included the data request. | ||
| let timestamp = { |
There was a problem hiding this comment.
should it be in a function?
|
Do you return the timestamp according to block explorer way (+1 epoch)? |
No, because this should be the timestamp of when the commit transaction was created, not the timestamp of the block that includes that transaction. |
According with a TODO comment in the wit_poller, we are using the timestamp when the request was included, so using the timestamp when the block that includes the data request is included in the blockchain could be nearer to the timestamp when the commit transaction was created, what do you think? |
tmpolaczyk
force-pushed
the
bridge-batch-report
branch
from
d6d5074
to
9303fce
Compare
Ok, I changed the block epoch from "the block that included the data request" to "that + 1", which will be more accurate, but it is still wrong in cases when there is more than one commit round. |
lrubiorod
dismissed
aesedepece’s stale review
Requested changes already included
Co-authored-by: Tomasz Polaczyk <[email protected]>
And refactor get_timestamp_from_block into a function
Using the same strategy as in the node
tmpolaczyk
force-pushed
the
bridge-batch-report
branch
from
5570c9f
to
4c16477
Compare
Close #2163
Fix #2046
Fix #2131