Skip to content

chore(sync/types): add sync/types package (syncer contract types) #4405

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.

Sign up for GitHub

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

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

chore(sync/types): add sync/types package (syncer contract types) #4405

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
772bfe1
chore(sync/types): add sync/types package that contains the main Sync…
powerslider Oct 8, 2025
ee84b72
Merge branch 'master' into powerslider/4404-syncer-interface-types
powerslider Oct 14, 2025
41ceeb7
Merge branch 'master' into powerslider/4404-syncer-interface-types
powerslider Oct 14, 2025
09ea80d
Merge branch 'master' into powerslider/4404-syncer-interface-types
powerslider Oct 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions vms/evm/sync/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

vms/evm/sync/types/types.go

This is in the correct folder? what other types will be in sync/types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the original code. Probably I'll add only SummaryProvider in a later PR when I move the code that will use it. Only the Extender interface will not be migrated for now, because we will not migrate the atomic syncer (which uses it).

// See the file LICENSE for licensing terms.

package types
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me why this package exists - it doesn't have the same name, but it feels like an "interfaces" package which is an anti-pattern. This is only used in coreth's vmsync package for the common syncer types defined in sync/statesync. Would it make sense to move to one of those two packages? Go typically defines interfaces in the consumer but if there's a well-agreed upon abstraction and multiple packages are using it as an interface can be reasonable to define it in the producer.

On an unrelated note... why is vmsync called vmsync? It seems like we named it that way to disambiguate against sync, but you can use import aliasing to get around this when there is a conflict.

Copy link
Contributor Author

@powerslider powerslider Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I'll start addressing all your points one by one:

  • The state sync code in coreth is mostly split in two places roughly: plugin/evm and the root sync package:
  • plugin/evm/vmsync is named this way because I needed to migrate this code initially from the root sync package in coreth to plugin/evm. I am not saying that this is the best name ever, but it made sense given the structure in coreth` at least temporarily.
  • How I envision the new structure to look like is the following:
 # avalanchego project
 
 vms
    evm
       sync
          block
             syncer.go -> implements the Syncer interface
          code
             syncer.go -> implements the Syncer interface
          state
             syncer.go -> implements the Syncer interface
          types
             types.go - > contains the Syncer interface
          registry (up for debate)
             ... all files from the coreth vmsync package

#coreth project

plugin
   evm
     atomic
       sync
          syncer.go -> imports the Syncer interface from avalanchego and implements it
          # This will import "github.com/ava-labs/avalanchego/vms/evm/sync/types"

PS: I know that the coreth structure is really bad and the idea is to finally unify it under vms/evm/sync in avalanchego. For now the atomic syncer will remain in coreth based on the decision we made on the meetings beforehand. So we also need the flexibility of importing the Syncer interface that will be in avalanchego now in coreth as well.


import "context"

// Syncer is the common interface for all sync operations.
// This provides a unified interface for atomic state sync and state trie sync.
type Syncer interface {
// Sync completes the full sync operation, returning any errors encountered.
// The sync will respect context cancellation.
Sync(ctx context.Context) error

// Name returns a human-readable name for this syncer implementation.
Name() string

// ID returns a stable, machine-oriented identifier (e.g., "state_block_sync", "state_code_sync",
// "state_evm_state_sync", "state_atomic_sync"). Implementations should ensure this is unique and
// stable across renames for logging/metrics/deduplication.
ID() string
Comment on lines +15 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

The caller here already knows about the concrete syncer types being registered, so we don't need to make this part of the interface public contract. Even if we did, it feels like overkill that we're adding this to the interface for duplicate handling because we own the syncers that we're registering - so it shouldn't happen anyways. It also feels leaky that something that just knows about how to perform a sync operation also needs to define its own metrics namespace and it's own "human-readable name".

}