Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/chains/evm/client: eth_call: include duplicate legacy field for compatability #12140

Merged
merged 1 commit into from
Feb 24, 2024

Conversation

jmank88
Copy link
Contributor

@jmank88 jmank88 commented Feb 22, 2024

No description provided.

Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

}
if len(msg.Data) > 0 {
arg["input"] = hexutil.Bytes(msg.Data)
arg["data"] = hexutil.Bytes(msg.Data) // duplicate legacy field for compatability
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting - so geth included these before and has now removed it in the latest version. Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The server for a long time has accepted both (>6 years?), and they recently switched the client from data to input.


// COPIED FROM go-ethereum/ethclient/gethclient - must be kept up to date!
Copy link
Contributor

Choose a reason for hiding this comment

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

What commit sha of go-ethereum was this copied from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one that is imported in the go.mod


// COPIED FROM go-ethereum/ethclient/gethclient - must be kept up to date!
// Modified to include legacy 'data' as well as 'input' in order to support non-compliant servers.
func toCallArg(msg ethereum.CallMsg) interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jmank88 ethereum recently updated this function to and added extra fields, should we reflect those changes?

https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L642

Copy link
Contributor

@stackman27 stackman27 Feb 23, 2024

Choose a reason for hiding this comment

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

nvm 1.13.8 doesnot use it, just need to ensure we update this function if we do modify to new geth version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied from the version we import to minimize changes

stackman27
stackman27 previously approved these changes Feb 23, 2024
Copy link
Contributor

@stackman27 stackman27 left a comment

Choose a reason for hiding this comment

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

LGTM

@jmank88 jmank88 added this pull request to the merge queue Feb 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2024
@snehaagni snehaagni added this pull request to the merge queue Feb 24, 2024
Merged via the queue into develop with commit 019b0c2 Feb 24, 2024
97 checks passed
@snehaagni snehaagni deleted the eth_call-input-data branch February 24, 2024 02:39
jinhoonbang pushed a commit that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants