alloy-rs / alloy

Transports, Middleware, and Networks for the Alloy project
https://alloy.rs
Apache License 2.0
653 stars 236 forks source link

fix(consensus): transaction types should be able to deserialize old name #1655

Closed dbeal-eth closed 3 days ago

dbeal-eth commented 5 days ago

Motivation

the gas_limit field in the transaction types was recently renamed to serialize to gas instead of gasLimit

However, this causes old state dumps prior to this change to fail to load (in fact, downstream foundry tests intended to prevent these types of breaking changes were modified in order to accommodate for this, although I really don't think this should have happened) https://github.com/foundry-rs/foundry/commit/7b118faeff4848be480f9c30c11237b9e9e6eb31 .

Solution

added alias so that serde knows it can also read from gasLimit if it doesn't find gas field.

manually verified that downstream foundry seems to work better with the legacy tests reverted to previous after making these changes.

PR Checklist

Other

Regarding tests, the downstream foundry tests should be reverted to their previous state (https://github.com/foundry-rs/foundry/commits/7b118faeff4848be480f9c30c11237b9e9e6eb31/crates/anvil/test-data/state-dump-legacy.json and https://github.com/foundry-rs/foundry/commits/7b118faeff4848be480f9c30c11237b9e9e6eb31/crates/anvil/test-data/state-dump-legacy-stress.json) once this change gets downstreamed to verify the change. However, in this repo, it seems we don't yet have serialization tests that verify decoding from json or other similar language. is there a reccomended way to add this?

yash-atreya commented 3 days ago

Resolved by #1654

dbeal-eth commented 3 days ago

Resolved by #1654

@yash-atreya thanks for your attention on this!

do you know if the tests in the downstream foundry repo will be reverted back to their previous state? This issue would not have been propogated in the first place had the tests not been modified.