-
Notifications
You must be signed in to change notification settings - Fork 809
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.
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
base: master
Are you sure you want to change the base?
Conversation
…er interface resolves #4404 Signed-off-by: Tsvetan Dimitrov ([email protected])
@@ -0,0 +1,22 @@ | |||
// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved. |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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).
// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved. | ||
// See the file LICENSE for licensing terms. | ||
|
||
package types |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 rootsync
package:- sync/types.go contains the interfaces that are implemented across both places. The reason why they are separated is because we don't want to have cyclic imports. The
Syncer
interface is implemented by:- sync/statesync/state_syncer.go (for the HashDB state syncer)
- sync/statesync/blocksync/syncer.go (for the block syncer)
- sync/statesync/code_syncer.go (for the code syncer)
- plugin/evm/atomic/sync/syncer.go (for the atomic syncer)
- sync/types.go contains the interfaces that are implemented across both places. The reason why they are separated is because we don't want to have cyclic imports. The
- plugin/evm/vmsync is named this way because I needed to migrate this code initially from the root
sync
package incoreth 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.
// 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 |
There was a problem hiding this comment.
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".
Why this should be merged
Check #4404
How this works
Syncer
interface type that will be the main contract for all syncer implementations migrated in follow up PRs.How this was tested
existing UT
Need to be documented in RELEASES.md?
no
resolves #4404
Signed-off-by: Tsvetan Dimitrov ([email protected])