[fortuna] Add confirmation delay before responding #1161
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
| @@ -1,19 +1,29 @@ | |||
| [ | |||
| { | |||
| "type": "constructor", | |||
| "inputs": [ | |||
| "type": "function", | |||
There was a problem hiding this comment.
we hadn't regenerated this in a while so there are unrelated ABI changes mixed in here.
| @@ -35,7 +34,7 @@ pub struct RegisterProviderOptions { | |||
| /// The fee to charge (in wei) for each requested random number | |||
| #[arg(long = "pyth-contract-fee")] | |||
| #[arg(default_value = "100")] | |||
| pub fee: U256, | |||
| pub fee: u128, | |||
There was a problem hiding this comment.
this is fallout from updating the ABI. this changed on-chain in a previous pr and didn't get updated here
fortuna/src/api/revelation.rs
Outdated
|
|
||
| match maybe_request { | ||
| Some(_) => { | ||
| Some(r) if current_block_number - state.confirmation_blocks >= r.block_number => { |
There was a problem hiding this comment.
it shouldn't happen but let's make it saturating sub to be extra careful
There was a problem hiding this comment.
The code looks correct.
However, this optimization to embed something in an existing type freaks me out and increases the likelihood of making mistakes in the future. I am not sure it is entirely worth it but you will know better. The least thing is changing the name of the variable everywhere so it doesn't get mistaken by just a blockNumber.
fortuna/src/chain/ethereum.rs
Outdated
| @@ -194,9 +197,15 @@ impl EntropyReader for PythContract { | |||
| Ok(Some(reader::Request { | |||
| provider: r.provider, | |||
| sequence_number: r.sequence_number, | |||
| block_number: r.block_number.abs().try_into()?, | |||
There was a problem hiding this comment.
To support my comment. This abs here is so subtle (and without docs) that can get easily missed if someone edits this code.
There was a problem hiding this comment.
Would suggest struct BlockHeight() with a validating constructor to make this never slip through, in general I'd advocate introducing more types and avoiding type in general.
fortuna/src/api/revelation.rs
Outdated
|
|
||
| let (maybe_request, current_block_number) = | ||
| try_join!(maybe_request_fut, current_block_number_fut) | ||
| .map_err(|_| RestError::TemporarilyUnavailable)?; |
There was a problem hiding this comment.
Minor point but it might be nice to log which of these failed by logging the error just in case.
fortuna/src/config.rs
Outdated
| @@ -115,6 +118,9 @@ pub struct EthereumConfig { | |||
| /// Address of a Pyth Randomness contract to interact with. | |||
| pub contract_addr: Address, | |||
|
|
|||
| /// How many blocks to wait before revealing the random number. | |||
| pub confirmation_blocks: BlockNumber, | |||
There was a problem hiding this comment.
Very very minor nitpick and possibly bikeshedding, but I think the name is a bit confusing, reveal_delay_blocks might be a bit better.
There was a problem hiding this comment.
yeah agree with this. renamed it.
fortuna/src/chain/ethereum.rs
Outdated
| @@ -194,9 +197,15 @@ impl EntropyReader for PythContract { | |||
| Ok(Some(reader::Request { | |||
| provider: r.provider, | |||
| sequence_number: r.sequence_number, | |||
| block_number: r.block_number.abs().try_into()?, | |||
There was a problem hiding this comment.
Would suggest struct BlockHeight() with a validating constructor to make this never slip through, in general I'd advocate introducing more types and avoiding type in general.
This PR adds a configurable confirmation delay to fortuna for each blockchain. The purpose of this delay is to ensure that the request has confirmed and will not be rolled back in a chain reorg before revealing the random number.
In order to implement this change, I had to update the entropy contract to store the block number regardless of whether or not the blockhash was used. I did this using 1 bit of the number to save storage slots (sigh).