Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

FIX: Accept 'input' or 'data' or both in transaction request #460

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Dec 7, 2023

Equivalent of filecoin-project/lotus#11471

Introduces an Eip1559TransactionRequestCompat type next to our TypedTransactionRequestCompat which we use to deserialize JSON requests. It accepts input or data, preferring input, before being mapped to the ethers type.

Instead of adding both fields I used the approach in I found here.

I also tried with #[serde(rename="input", alias = "data")] but that does not accept duplicate fields.

Eventually ended up with Raul's suggestion of adding input but using flatten to wrap the original, which requires the minimum amount of code.

Also opened a PR in ethers but ended up only adding an alias = "input" to keep things simple. I think the trend is that clients will only send one value or the other, at least the double sending is being fixed in web3.js, so the added complexity of supporting both fields seems like an overkill. We can keep our Compat stuff to support both fields though.

@aakoshh
Copy link
Contributor Author

aakoshh commented Dec 7, 2023

Opened an upstream PR: gakonst/ethers-rs#2697

Copy link
Contributor

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but would add input parsing for all transaction types supported by Fendermint.

fendermint/eth/api/src/apis/eth.rs Outdated Show resolved Hide resolved
fendermint/eth/api/src/apis/eth.rs Outdated Show resolved Hide resolved
@aakoshh aakoshh merged commit 1841a4f into main Dec 8, 2023
5 checks passed
@aakoshh aakoshh deleted the fix-eth-input-vs-data branch December 8, 2023 11:51
Copy link
Contributor

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final implementations is basically what I had locally, so post factum LGTM again.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants