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

fix: handle SyncingStatus from erigon #2700

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tsbernar
Copy link

Motivation

The syncing() eth_syncing rpc call fails on Erigon clients when the node is not synced. Erigon gives a different response format that we fail to parse. This change adds handling for the Erigon response format: https://github.com/ledgerwatch/erigon/blob/6a1bb1dff1deca7a50d66bfbc9e0b89bb4e335a1/turbo/jsonrpc/eth_system.go#L70-L74

Solution

Modifies the SyncProgress struct to handle all responses, adds a unit test.

PR Checklist

  • Added Tests
  • Added Documentation - just code comments
  • Breaking changes

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

sorry for the late review

this does not match the "spec", or rather breaks it with the optional starting_block field, which is not great

ethers-core/src/types/syncing.rs Outdated Show resolved Hide resolved
@tsbernar
Copy link
Author

sorry for the late review

this does not match the "spec", or rather breaks it with the optional starting_block field, which is not great

Thanks for the review @mattsse.
What do you think the right approach for this would be if not making it optional? Maybe some other pattern of having opt-in client specific types? I was just trying to be compatible with erigon from here:

https://github.com/ledgerwatch/erigon/blob/6a1bb1dff1deca7a50d66bfbc9e0b89bb4e335a1/turbo/jsonrpc/eth_system.go

Erigon is quite popular so I don't think we should be unable to parse responses from this client.

@mattsse
Copy link
Collaborator

mattsse commented Feb 13, 2024

I think it's fine

only thing we need is pub fields

@tsbernar
Copy link
Author

I think it's fine

only thing we need is pub fields

Hey @mattsse, I've made that update now

@tsbernar tsbernar requested a review from mattsse March 30, 2024 23:11
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.

None yet

2 participants