Dynamic Contract Pre Registration #252
Conversation
JonoPrest
force-pushed
the
jp/contract-preregistration
branch
from
3ad7e16
to
952732b
Compare
|
It doesn't break anything on the hosted service. Will need to adapt the UI a bit to represent the pre registration step. |
| } | ||
|
|
||
| @genType | ||
| type eventConfig<'eventFilter> = { | ||
| wildcard?: bool, | ||
| eventFilters?: SingleOrMultiple.t<'eventFilter>, | ||
| preRegister?: bool, |
There was a problem hiding this comment.
| preRegister?: bool, | |
| shouldPreRegisterDynamicContracts?: bool, |
(and everywhere else)
I think it is good to have consistent naming on this. I assume you want it to be less verbose in the config and more verbose in the code. Maybe then we just use preregister everywhere instead of the longer variable. I don't think it is unclear to just be preRegister
There was a problem hiding this comment.
IMO I prefer the verbose naming since it's quite an esoteric feature.
The shortened version I just used for the user API since I don't think they should have such a verbose flag. In the same way, we use isWildcard for our internal config everywhere but for the user API it's simply wildcard
I would be open to renaming the the user facing config flag but would prefer to keep the verbose name everywhere else.
There was a problem hiding this comment.
Cool, maybe middle ground - preRegisterDynamicContracts everywhere?
I don't really agree that the variable will be confusing, especially since the variable comes from the user's handlers and will be in our box, etc., but I think we can make it more descriptive
There was a problem hiding this comment.
@JasoonS for the user API we have Contract.Event.contractRegister so maybe even preRegisterContracts I was thinking?
Still prefer verbose naming with "should" or "is" prefix in internal code to imply boolean type.
| @@ -316,7 +317,12 @@ module Make = ( | |||
| ) | |||
|
|
|||
| //Instantiate each time to add new registered contract addresses | |||
| let recieptsSelection = getRecieptsSelection(~contractAddressMapping, ~shouldApplyWildcards) | |||
| let recieptsSelection = if isPreRegisteringDynamicContracts { | |||
There was a problem hiding this comment.
Hmm, is there no way to catch this at codegen time? I guess the user will get notified as soon as it starts. So, fine.
There was a problem hiding this comment.
nvm - it is temprorary too.
There was a problem hiding this comment.
No since the config is set with the js API
There was a problem hiding this comment.
Cool, I think the only real thing is to change the variable to pre-register everywhere, even internally. The code seems really good. I didn't notice any bugs or anything.
It is quite a bit to follow though, so I don't fully trust myself to have caught bugs - but I think we can start testing it in the wild.
| @@ -810,7 +965,6 @@ let injectedTaskReducer = ( | |||
| //On the first time we enter the reorg threshold, copy all entities to entity history | |||
| //And set the isInReorgThreshold isInReorgThreshold state to true | |||
| dispatchAction(SetIsInReorgThreshold(true)) | |||
| //TODO: persist the isInReorgThreshold state to the db in a transaction with copy | |||
There was a problem hiding this comment.
It's implemented 👍🏼
| @@ -130,7 +134,7 @@ let make = (~chainData: chainData) => { | |||
| loaded={latestProcessedBlock - firstEventBlockNumber} | |||
| buffered={latestFetchedBlockNumber - firstEventBlockNumber} | |||
| outOf={toBlock - firstEventBlockNumber} | |||
| loadingColor=Secondary | |||
| loadingColor={isPreRegisteringDynamicContracts ? Primary : Secondary} | |||
| @@ -41,6 +41,22 @@ type chainConfig = { | |||
| chainWorker: module(ChainWorker.S), | |||
| } | |||
|
|
|||
| let shouldPreRegisterDynamicContracts = (chainConfig: chainConfig) => { | |||
There was a problem hiding this comment.
I think this function name is fine, but then change the variable to just preRegister (as the user sees it)
Co-authored-by: Jason Smythe <[email protected]>
JonoPrest
force-pushed
the
jp/contract-preregistration
branch
from
b4add71
to
c8418e6
Compare
Implements Dynamic Contract Preregistration
Before this gets merged for a release, we need to validate how the change affects the hosted service UI since events sync state table has been updated