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

refactor: add StateSync component #646

Closed
wants to merge 5 commits into from
Closed

Conversation

tomyrd
Copy link
Collaborator

@tomyrd tomyrd commented Dec 23, 2024

Objective

This PR is a proof of concept for the first client component. The idea of components is that they are parts of the client's logic that users can compose and add their own functionality.
These components could be passed to the client's constructor like the store and rpc (this is not implemented in this draft but it wouldn't be hard to do so).

In this initial iteration, a new StateSync component is added that implements the old sync_state_once function. In terms of additional logic, there isn't much, the old code is moved to this new component and the interface is declared in a trait so that users can create their own.

Example

While implementing this I wanted to add an example of a possible alternate StateSync component someone could implement. I remembered we have a simpler sync (what we call note sync) inside the import note logic. I created a StateSync in import.rs that only looks for updates of a specific note record. After implementing it though, I don't think this example is adequate tough, as the component isn't used how an external user would use it (passing it to the client as a constructor). It does show how the interface could be used in different ways. (this "example" is contained in the last commit so it can be ignored in the review tab). Before fixing the example so it's used in the intended way I had some thoughts I wanted to discuss.

Some thoughts before continuing

Before continuing with the implementation (tidying up the code organization, etc.) I wanted to create this draft PR to further discuss this initial iteration of the component feature. Some comments I had during the process:

  • I had trouble thinking of possible ways an external user would want to change the sync logic. I feel like the whole process is too specific to the client and there isn't much someone could change. Even the example implementation is just a stripped down version, only looking for one note.
  • Another approach would be for the component to implement various small functions instead of the big sync process. Maybe one for each internal sync update (notes, accounts, transactions, mmr paths) and the client could use them. These maybe have the problem of being too restrictive, how many ways for updating an account are there?
  • I understand that the idea is not to create this varying implementations ourselves, but to give the possibility for users to modify the client in ways we cannot think of, so maybe it's OK to not know how the components could be used.
  • Finally, I think the most important question would be: Is the client flexible enough to allow for varying implementations for each internal process? I fear that in changing the client and its processes to be ever more generic, we introduce a lot of unwanted complexity.

Maybe I'm overthinking it and this way of implementing it isn't ideal. I tried different ways, but I thought this one was the simplest.

Comment on lines +113 to +119
/// The [SyncState] trait defines the interface for the state sync process. The sync process is
/// responsible for updating the client's store with the latest state of the network.
///
/// Methods in this trait shouldn't change the state of the client itself, but rather return the
/// updated state as a result.
#[async_trait(?Send)]
pub trait SyncState {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've only looked at this briefly, so could be missing the main idea entirely - but I was thinking of components a bit differently. Specifically, the SyncState component would be a struct rather than a trait and it would be completely separate from the store. The workflow would look something like this then:

  1. The user instantiates SyncState struct by passing to it rpc_api and relevant initial data from the store.
  2. Then the user runs the sync() function.
  3. This function returns the data that the user would then filter through and insert into the store.

This SyncState component would then be used in our implementation of the Client, but users would be able to use it in other contexts too.

I think the main thing here is to define what data we need to provide to the StateSync component on initialization and what would be returned from the sync() function.

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 think I got a better idea. The user wouldn't create a different implementation, they just would get access to the same output the client will get on a sync. I like this approach better as it seems simpler and wouldn't change the client internals greatly. I'll create a separate branch for ths approach.

It would be completely separate from the store.

This function returns the data that the user would then filter through and insert into the store.

The returned information might be incomplete/not entirely processed, as the sync process involves getting some information from the store. Another option would be to get this needed information beforehand but maybe it would be wasteful.

Copy link
Contributor

Choose a reason for hiding this comment

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

The returned information might be incomplete/not entirely processed, as the sync process involves getting some information from the store. Another option would be to get this needed information beforehand but maybe it would be wasteful.

Yeah, I think one of the open questions is whether we can find a good balance here so that we don't have to pre-load too much info from the store, but also get some meaningful processing during the sync process.

@tomyrd tomyrd mentioned this pull request Jan 2, 2025
4 tasks
@tomyrd
Copy link
Collaborator Author

tomyrd commented Jan 10, 2025

replaced by #650

@tomyrd tomyrd closed this Jan 10, 2025
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.

2 participants