Colocation Notify - failed simulation #2013
Conversation
| from: tx.from.into(), | ||
| to: tx.to.into(), | ||
| data: tx.input.clone().into(), | ||
| internalization: InternalizationStrategy::SkipInternalizableInteraction, |
There was a problem hiding this comment.
I think the internalization flag should probably not be hardcoded. The simulation could also only fail if internalized AFAIK.
There was a problem hiding this comment.
I aimed here to actually drop the internalization flag as it is not needed IMO, since all solver engines are sending raw interactions to the driver and, in case of simulation failure, they get the failed transaction with calldata, so that would be enough to see if it was internalized or not (if the calldata in notification differs from what they sent to us, and they have solution id to match on).
So, I just want to be careful and add the bare minimum of data needed (to reduce complexity).
Here, I would rather add additional variant InternalizationStrategy::Undefined or similar, to make the code accurate and not misleading, instead of providing actual value.
WDYT?
There was a problem hiding this comment.
Added InternalizationStrategy::Unknown
There was a problem hiding this comment.
Sorry, saw the follow up comments too late.
Wouldn't it then make more sense to drop the internalization here altogether? Seems like in all the existing use cases having the Unknown variant doesn't really make sense and therefore compromising the use of an existing type for this doesn't seem worth it.
There was a problem hiding this comment.
Yes we can definitely drop it. Maybe I was too cautious to not cause any backward compatibility issues since this is after all a debugging feature.
There was a problem hiding this comment.
Tried removing but we are using this field to recreate tenderly link in the non-colocated driver also, so we will have to remove it only once the colocation is activated.
I added the comment in the code to remember this.
There was a problem hiding this comment.
I find it a bit strange that we introduce a few new identical Tx types rather than reusing eth::Tx.
I also don't understand the impact of populating the legacy notification with fake data for block_number and tx_index.
Except for this, I've nothing more to add.
|
Merging so I can observe it in staging before tomorrow colocation run in prod. @fedgiac feel free to add more comments |
Description
Implements (2) from #1891
Changes
driverto thesolvers.Unfortunately, not all data can (and should) be populated to support the legacy version.
block_numberis one of those that is needed but it is not provided by simulators. This is the never ending issue we generally have with doing simulations on a specific block number.The easiest fix would be to add get_block_number() call to each web3 simulation (enso and tenderly seem to give block_number in their responses), but it wouldn't be accurate.
At the moment I decided to skip it, since, after all, solvers can pretty much figure out the block number on their own, there are usually 1 or 2 candidates...