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

ethereum: fix GETH_ETH_CALL_ERRORS_ENV #2784

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

leoyvens
Copy link
Collaborator

@leoyvens leoyvens commented Sep 9, 2021

Previously if GETH_ETH_CALL_ERRORS_ENV is not set, then any error returned from eth_call would be considered deterministic. This is because calling split on an empty string returns [""], not [] (playground), and all strings contain "". This affects release 0.24 and can cause subgraph failures on subgraphs using contract calls, and most severely incorrect data on subgraphs using try_ calls, which would return a reverted result when it fact it was a spurious error.

This did not affect the hosted service because it has GETH_ETH_CALL_ERRORS_ENV set to "out of gas".

@leoyvens leoyvens force-pushed the leo/fix-GETH_ETH_CALL_ERRORS_ENV branch from 858b84c to c62b751 Compare September 9, 2021 15:47
Copy link
Contributor

@evaporei evaporei left a comment

Choose a reason for hiding this comment

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

Wow, I didn't know about this.

@leoyvens leoyvens force-pushed the leo/fix-GETH_ETH_CALL_ERRORS_ENV branch from c62b751 to 29900cd Compare September 9, 2021 16:34
Previously if `GETH_ETH_CALL_ERRORS_ENV` is not set, then any error
returned from eth_call would be considered deterministic. This is
because calling `split` on an empty string returns `[""]`, not `[]`,
and all strings contain `""`. This affects release 0.24 and can
cause subgraph failures on subgraphs using contract calls,
and most severely incorrect data on subgraphs using `try_` calls.
@leoyvens leoyvens force-pushed the leo/fix-GETH_ETH_CALL_ERRORS_ENV branch from 29900cd to ed28b2f Compare September 9, 2021 17:37
@vrmiguel
Copy link
Contributor

vrmiguel commented Sep 9, 2021

Wouldn't split_terminator fix the issue?

    let foo: Vec<_> = String::new()
        .split_terminator(';')
        .map(ToOwned::to_owned)
        .collect();
     
    // Prints `[]`
    println!("{:?}", foo);
    
    let abc: Vec<_> = "A;B;C"
        .split_terminator(';')
        .map(ToOwned::to_owned)
        .collect();

    // Prints `["A", "B", "C"]`
    println!("{:?}", abc);

@leoyvens
Copy link
Collaborator Author

leoyvens commented Sep 9, 2021

@vrmiguel that would work better, good to know. This is all very confusing, so I've settled for just explicitly filtering out empty strings.

.split(';')
.map(ToOwned::to_owned)
.collect()
.map(|s| s.split(';').filter(|s| s.len() > 0).map(ToOwned::to_owned).collect())
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually s.is_empty() is a better and clearer option then s.len() > 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I always have a hard time reasoning over negated conditionals, so I prefer s.len() > 0 over !s.is_empty().

@leoyvens leoyvens merged commit 3dff0d8 into master Sep 9, 2021
@evaporei evaporei deleted the leo/fix-GETH_ETH_CALL_ERRORS_ENV branch September 9, 2021 18:32
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.

3 participants