-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[BCI-3989][core] - CR methods err when service unstarted #14123
Draft
Farber98
wants to merge
80
commits into
develop
Choose a base branch
from
BCI-3989-cr-methods-error-when-unstarted
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 59 commits
Commits
Show all changes
80 commits
Select commit
Hold shift + click to select a range
111057c
add isStarted check
Farber98 6f8ed23
move start outside getChainReader
Farber98 0074a33
bump common version
Farber98 2132bf0
Squashed commit of the following:
Farber98 53adad8
Revert "Squashed commit of the following:"
Farber98 bf588c7
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 2cce774
bump common version
Farber98 e8b366e
fix ccip failing integration tests
Farber98 7fa91e6
Revert "fix ccip failing integration tests"
Farber98 ed83615
remove check from bind
Farber98 fa2d416
bump common version
Farber98 4b2e503
fix chain reader tests
Farber98 7184150
StartChainReader method
Farber98 1e41db4
bump common version
Farber98 add59d8
start chain reader inside registry syncer
Farber98 db99f56
Revert "start chain reader inside registry syncer"
Farber98 7fde790
make error more clear for logging
Farber98 83c2559
starting cr service in write capability
Farber98 52e3313
starting contractReader inside registrySyncer
Farber98 b0c3305
idempotency if already started or already closed
Farber98 46d0c84
start reader in ccip delegate
Farber98 deb2d30
create var for err
Farber98 8a7054d
bump common version
Farber98 2aa2ee3
close chain reader after tests
Farber98 c63bcc6
bump common version
Farber98 733e0e0
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 894835e
bump common version
Farber98 d7153ac
add changeset
Farber98 be1d2c8
use state machine instead of flag
Farber98 46d7e32
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 4ea601d
use state machine ready method
Farber98 19a0268
idempotent start and close
Farber98 a0344e4
rename methods + bump common version
Farber98 5f45f15
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 482bb8b
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 5c93499
bump common version
Farber98 0e8f75f
bump common
Farber98 2eb9b0d
use state machine helpers to check if started inside methods
Farber98 6381bcd
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 f8d149d
bump common version
Farber98 160ea54
fix conflict
Farber98 0272eec
fix conflict
Farber98 d945744
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 017380c
remove start and close state check
Farber98 7448064
separate instantiation from starting svc in syncer tests
Farber98 6fc175f
remove Start and return svc started from getChainReader
Farber98 2a236da
bump common
Farber98 e42f6dd
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 3bff641
bump deps
Farber98 917add2
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 732f4c0
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 ca2124d
bump common
Farber98 19ac700
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 f13379f
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 ec4dcbd
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 3684bde
bump common
Farber98 eaa8149
fix tidy & generate ci
Farber98 e06b1d1
fix go.md error
Farber98 d6e2d43
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
ilija42 cadbc0f
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 2715354
bump common
Farber98 9b8be2f
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 8ddc56c
add start. add start and close where needed
Farber98 8095ff8
bump common
Farber98 872fd3c
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 00044c6
bump common
Farber98 e1792e9
merge develop
Farber98 76ed985
merge develop
Farber98 218f3cd
merge develop
Farber98 d8e1c2b
fix conflicts with chain components pr
Farber98 37c892c
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 3fcbbf6
fix conflicts pr
Farber98 56b0f5f
bump common
Farber98 e569a69
add flag to control if we return cr started
Farber98 36e4489
bump common
Farber98 c83a850
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 40d2cc1
bump common
Farber98 e96d20c
refactor codec and chaincomponents ifaces
Farber98 07c8f01
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 b22ad6b
bump common
Farber98 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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 |
---|---|---|
|
@@ -75,7 +75,7 @@ require ( | |
github.com/smartcontractkit/chain-selectors v1.0.21 | ||
github.com/smartcontractkit/chainlink-automation v1.0.4 | ||
github.com/smartcontractkit/chainlink-ccip v0.0.0-20240828115624-442f1cff195b | ||
github.com/smartcontractkit/chainlink-common v0.2.2-0.20240829145110-4a45c426fbe8 | ||
github.com/smartcontractkit/chainlink-common v0.2.2-0.20240830171115-699bdf8c4aae | ||
github.com/smartcontractkit/chainlink-cosmos v0.4.1-0.20240710121324-3ed288aa9b45 | ||
github.com/smartcontractkit/chainlink-data-streams v0.0.0-20240820130645-cf4b159fbba2 | ||
github.com/smartcontractkit/chainlink-feeds v0.0.0-20240710170203-5b41615da827 | ||
|
@@ -279,7 +279,6 @@ require ( | |
github.com/modern-go/reflect2 v1.0.2 // indirect | ||
github.com/mostynb/zstdpool-freelist v0.0.0-20201229113212-927304c0c3b1 // indirect | ||
github.com/mtibben/percent v0.2.1 // indirect | ||
github.com/mwitkow/grpc-proxy v0.0.0-20230212185441-f345521cb9c9 // indirect | ||
github.com/oklog/run v1.1.0 // indirect | ||
github.com/opencontainers/go-digest v1.0.0 // indirect | ||
github.com/opentracing/opentracing-go v1.2.0 // indirect | ||
|
@@ -294,6 +293,7 @@ require ( | |
github.com/sasha-s/go-deadlock v0.3.1 // indirect | ||
github.com/sethvargo/go-retry v0.2.4 // indirect | ||
github.com/shirou/gopsutil v3.21.11+incompatible // indirect | ||
github.com/smartcontractkit/grpc-proxy v0.0.0-20240830132753-a7e17fec5ab7 // indirect | ||
github.com/spf13/afero v1.9.5 // indirect | ||
github.com/spf13/cobra v1.8.0 // indirect | ||
github.com/spf13/jwalterweatherman v1.1.0 // indirect | ||
|
@@ -349,10 +349,5 @@ require ( | |
sigs.k8s.io/yaml v1.4.0 // indirect | ||
) | ||
|
||
replace ( | ||
// replicating the replace directive on cosmos SDK | ||
github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1 | ||
|
||
// until merged upstream: https://github.com/mwitkow/grpc-proxy/pull/69 | ||
github.com/mwitkow/grpc-proxy => github.com/smartcontractkit/grpc-proxy v0.0.0-20230731113816-f1be6620749f | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems it's not needed anymore as it has been replaced by |
||
) | ||
// replicating the replace directive on cosmos SDK | ||
replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1 |
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.
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) error
callsfunc (s *registrySyncer) syncLoop()
that callsfunc (s *registrySyncer) Sync(ctx context.Context, isInitialSync bool) error
which 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
newReader
func 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
CR
service to be inStarted
state before calling the methods. Thissyncer
was 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
Started
before anything that uses them, then there is no need to be defensive about checking internally ifStart
has been called. But I see that there is a good reason to defer in this case.