Add generic ChainWorker module - #78
Conversation
DZakh
left a comment
DZakh
left a comment
There was a problem hiding this comment.
In general, nothing important. But I'd say it makes the project more maintainable.
| type source<'a, 'b> = HyperSync('a) | Rpc('b) | ||
|
|
||
| type syncSource = source<serverUrl, rpcConfig> | ||
| type syncSource = HyperSync(hyperSyncConfig) | HyperFuel(hyperFuelConfig) | Rpc(rpcConfig) |
There was a problem hiding this comment.
HyperFuel is not used yet. The option fullbacks to HyperSyncWorker
| chainConfig, | ||
| rpcConfig, | ||
| config, | ||
| let currentBlockInterval = ref(T.rpcConfig.syncConfig.initialBlockInterval) |
There was a problem hiding this comment.
!!! Important: This has become mutable. That should be fine for the case, but I want to double check with you.
There was a problem hiding this comment.
Your changes shouldn't change current behaviour but I think with the new partition system, this value should be stored on the partition because they can be fetching very different volumes of data. Or perhaps just have a worker per partition.
| @as("latest_processed_block") latestProcessedBlock: option<int>, | ||
| @as("num_events_processed") numEventsProcessed: option<int>, | ||
| @as("is_hyper_sync") isHyperSync: bool, | ||
| @as("is_hyper_sync") poweredByHyperSync: bool, |
There was a problem hiding this comment.
This is going to require changes to the UI & being able to be backwards compatible to query the "is_hyper_sync". Since this table is used for all the sync/progress bars on the hosted service.
It's best not to change this unless we need to to 👍🏼
There was a problem hiding this comment.
Yeah, I've seen it in the code. The change is fine, since the @as("is_hyper_sync") stays the same
There was a problem hiding this comment.
I was just confused what the variable needed for, so after figuring it out I've decided to rename it on the ReScript side, to make it more readable (in my opinion)
There was a problem hiding this comment.
Ah got you! I didn't see the @as
| open ChainWorker | ||
|
|
||
| module Make = ( | ||
| T: { | ||
| let config: Config.t | ||
| let rpcConfig: Config.rpcConfig | ||
| let chainConfig: Config.chainConfig | ||
| }, | ||
| ): Type => { | ||
| let name = "RPC" | ||
| let config = T.config | ||
| let rpcConfig = T.rpcConfig | ||
| let chainConfig = T.chainConfig | ||
| let chain = chainConfig.chain |
There was a problem hiding this comment.
Out of interest what made you want to use a functor for this with static variables?
There was a problem hiding this comment.
Otherwise I'd have to keep state and worker mod separately, and I couldn't figure out how to type them nicely, to tell that they belong to each other. So I've decided it's better to simply have only worker mod
| worker: Config.source<'a, 'b>, | ||
| } | ||
|
|
||
| module type Type = { |
There was a problem hiding this comment.
Just FYI it is idiomatic to call type signature of a Functor module type S, like using type t for a module's main type.
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Cool Dmitry,
Overall, I like the concept passing around the first class modules to allow mixed types 🙂 I used to do something similar where ChainWorker actually just had a set of functions that would take the variant Rpc(worker) | Hypersync(worker) and then call the appropriate function. But it was getting bloated for the little it was used.
What I'm not a huge fan of is that you end up passing around objects that contain functions rather than just values. Reminds me a lot of classes/OOP 🤔
I think lets also just think through the repercussions of renaming that field in chain metadata.
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Looks great Dmitry! 🚀
2 participants
No description provided.