[BCI-3989][core] - CR methods err when service unstarted #14123
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "chainlink": minor | ||
| --- | ||
|
|
||
| chainReader methods return err when called and service is not started yet. #internal |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,10 +40,12 @@ type chainReader struct { | |
| bindings | ||
| codec commontypes.RemoteCodec | ||
| commonservices.StateMachine | ||
| isStarted bool | ||
|
There was a problem hiding this comment. You should be able to access the started state in the There was a problem hiding this comment. Thanks for this! Looks cleaner. I was trying to replicate the way txm components do this There was a problem hiding this comment. addressed here: |
||
| } | ||
|
|
||
| var _ ChainReaderService = (*chainReader)(nil) | ||
| var _ commontypes.ContractTypeProvider = &chainReader{} | ||
| var errServiceNotStarted = errors.New("ContractReader service not started") | ||
|
|
||
| // NewChainReaderService is a constructor for ChainReader, returns nil if there is any error | ||
| // Note that the ChainReaderService returned does not support anonymous events. | ||
|
|
@@ -141,31 +143,47 @@ func (cr *chainReader) Name() string { return cr.lggr.Name() } | |
|
|
||
| // Start registers polling filters if contracts are already bound. | ||
| func (cr *chainReader) Start(ctx context.Context) error { | ||
| if cr.isStarted { | ||
| return nil | ||
| } | ||
|
|
||
| return cr.StartOnce("ChainReader", func() error { | ||
| err := cr.bindings.ForEach(ctx, func(c context.Context, cb *contractBinding) error { | ||
| for _, rb := range cb.readBindings { | ||
| if err := rb.Register(ctx); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return cb.Register(ctx, cr.lp) | ||
| }) | ||
| if err == nil { | ||
| cr.isStarted = true | ||
| } | ||
| return err | ||
| }) | ||
| } | ||
|
|
||
| // Close unregisters polling filters for bound contracts. | ||
| func (cr *chainReader) Close() error { | ||
| if !cr.isStarted { | ||
| return nil | ||
| } | ||
|
|
||
| return cr.StopOnce("ChainReader", func() error { | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
| defer cancel() | ||
| err := cr.bindings.ForEach(ctx, func(c context.Context, cb *contractBinding) error { | ||
| for _, rb := range cb.readBindings { | ||
| if err := rb.Unregister(ctx); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return cb.Unregister(ctx, cr.lp) | ||
| }) | ||
| if err == nil { | ||
| cr.isStarted = false | ||
| } | ||
| return err | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -176,6 +194,10 @@ func (cr *chainReader) HealthReport() map[string]error { | |
| } | ||
|
|
||
| func (cr *chainReader) GetLatestValue(ctx context.Context, contractName, method string, confidenceLevel primitives.ConfidenceLevel, params, returnVal any) error { | ||
| if !cr.isStarted { | ||
| return errServiceNotStarted | ||
| } | ||
|
|
||
| b, err := cr.bindings.GetReadBinding(contractName, method) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -185,6 +207,10 @@ func (cr *chainReader) GetLatestValue(ctx context.Context, contractName, method | |
| } | ||
|
|
||
| func (cr *chainReader) BatchGetLatestValues(ctx context.Context, request commontypes.BatchGetLatestValuesRequest) (commontypes.BatchGetLatestValuesResult, error) { | ||
| if !cr.isStarted { | ||
| return nil, errServiceNotStarted | ||
| } | ||
|
|
||
| return cr.bindings.BatchGetLatestValues(ctx, request) | ||
| } | ||
|
|
||
|
|
@@ -193,6 +219,10 @@ func (cr *chainReader) Bind(ctx context.Context, bindings []commontypes.BoundCon | |
| } | ||
|
|
||
| func (cr *chainReader) QueryKey(ctx context.Context, contractName string, filter query.KeyFilter, limitAndSort query.LimitAndSort, sequenceDataType any) ([]commontypes.Sequence, error) { | ||
| if !cr.isStarted { | ||
| return nil, errServiceNotStarted | ||
| } | ||
|
|
||
| b, err := cr.bindings.GetReadBinding(contractName, filter.Key) | ||
| if err != nil { | ||
| return nil, err | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Shouldn't it be started in
func (s *registrySyncer) Start(ctx context.Context) error {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.
func (s *registrySyncer) Start(ctx context.Context) errorcallsfunc (s *registrySyncer) syncLoop()that callsfunc (s *registrySyncer) Sync(ctx context.Context, isInitialSync bool) errorwhich initializes the CR.I added these lines to start the CR service right after this initialization
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.
Why is it better to have the initialization here, rather than in
(*registrySyncer).Start? Isn't that just contributing to this problem in the first place?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.
What do you mean by
this problem?There's a note in the
newReaderfunc that says why this is achieved this way: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.
I added these lines as we are now enforcing
CRservice to be inStartedstate before calling the methods. Thissyncerwas not starting the service previously and that's why we got some tests failing but now they are passing with this changeThere 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.
If all dependencies are
Startedbefore anything that uses them, then there is no need to be defensive about checking internally ifStarthas been called. But I see that there is a good reason to defer in this case.