Combine onchain order event handling logic

[fleupold]

Description

When looking into adding a log statement when on chain orders are parsed (to debug time to happy moo), I noticed that the append_events and replace_events logic in the on chain order handler use exactly the same code except for the following block, which is only present in replace_events:

database::onchain_broadcasted_orders::mark_as_reorged(
    &mut transaction,
    *range.start() as i64,
)
.await
.context("mark_onchain_order_events failed")?;

database::onchain_invalidations::delete_invalidations(
    &mut transaction,
    *range.start() as i64,
)
.await
.context("invalidating_onchain_order_events failed")?;

Combining this logic remove code duplication and allows for easier adjusting of the logic.

Changes

• Replace the append_events logic by calling replace_events with an range that doesn't affect any orders.

How to test

CI

Additional Notes to Reviewers

Please double check my reasoning that replace_events indeed mirror the exact logic from append_events

[m-lord-renkse]

This can be problematic. We are passing here a u64::MAX, but then here we are unsafely casting it to a i64.

I am not so sure how the casting in Rust will affect here, but very likely it will parse to -1, affecting the range and affecting the database data.

[m-lord-renkse]

oh yeah it does: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ab66cb5d77df0b92469255eadbca8ed9

[fleupold]

Wow, very good catch

[m-lord-renkse]

Casting issue should be solved. Major impact into database/potential panic.

[fleupold]

I looked at better ways to work around this issue, but it looks like we store block number as a bigdecimal in the DB (which only support i64) but the main source of block number (ie. current block stream etc. return u64). So probably the cleanest change would be to alter the tables to store numeric instead of bigint

For now I replaced the unsafe cast with a safe one, but I'm not sure this is very safe going forward (I almost wiped the staging DB).

[m-lord-renkse]

@fleupold AFAIK, PostgreSQL does not explicitly support unsigned integer types 😞
We can create an internal type (with an i64 inside) which is forced to always be positive. And implement the sqlx derives so it can be used in the database 🤔

[m-lord-renkse]

What's more, we should maybe delete all the unsafe castings we have. It is very dangerous.
For types which have immediate casting (like u32 to u64), the From<> trait is already automatically derived (u64::from() vs x as u64), but these castings are fine with doing it with as.

For those who don't implement the trait From<> (because they do the TryFrom<> trait), then the unsafe casting should be definitely removed.

We could even have a CI to check for unsafe castings 🤔

[fleupold]

We could even have a CI to check for unsafe castings 🤔

Yes, I can follow up with a PR that adds a lint rule that enforces cast_possible_wrap = "deny".

For types which have immediate casting (like u32 to u64), the From<> trait is already automatically derived (u64::from() vs x as u64), but these castings are fine with doing it with as.

Hmh, I don't see a huge advantage of that over the proposed change (where we mindfully cast an overflow down to the largest possible i64, which should still be way above any reasonable block number).

[squadgazzz]

Looks legit

[m-lord-renkse]

IMO, in order to make it fully modular, we should have two functions delete_events() and append_events() (and replace_events() calls them both). Conceptually we are calling a replace_events() to append events, but we just use a range out of bounds so it doesn't replace any events, but the intention is not to replace events.

It is more of a concept/split of function nit.

[fleupold]

Good suggestion, I implemented this, but this changed the PR quite significantly. Therefore re-requesting review...