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.

Sign up for GitHub

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

Jump to bottom

bump libocr; add context #75

Merged
merged 1 commit into from
Oct 10, 2024
Merged

bump libocr; add context #75

merged 1 commit into from
Oct 10, 2024

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented Aug 22, 2024 •
edited
Loading

@jmank88 jmank88 mentioned this pull request Aug 22, 2024
Merged
@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch 5 times, most recently from 5c91b33 to 3ae36ba Compare September 3, 2024 16:18
@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch 10 times, most recently from bfe843c to cee56f6 Compare September 11, 2024 12:10
@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch 6 times, most recently from 9c2109c to 21f864a Compare September 19, 2024 08:57
@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch 8 times, most recently from d11cd36 to fac5f8c Compare September 24, 2024 13:33
@jmank88 jmank88 marked this pull request as ready for review October 9, 2024 13:44
@jmank88 jmank88 requested a review from a team as a code owner October 9, 2024 13:44
winder
winder reviewed Oct 9, 2024
View reviewed changes
execute/plugin.go Outdated Show resolved Hide resolved
@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch 2 times, most recently from e2f9254 to 6ff2fe8 Compare October 9, 2024 14:14
winder
winder reviewed Oct 9, 2024
View reviewed changes
commit/factory.go Show resolved Hide resolved
pkg/reader/usdc_reader.go Outdated Show resolved Hide resolved
@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch 2 times, most recently from 1abe360 to 1e4a7f6 Compare October 9, 2024 14:19
@jmank88 jmank88 requested a review from winder October 9, 2024 15:24
makramkd
makramkd requested changes Oct 10, 2024
View reviewed changes
commit/plugin.go Outdated Show resolved Hide resolved
@jmank88 jmank88 force-pushed the BCF-2887-context-propagation branch from 1e4a7f6 to 8910244 Compare October 10, 2024 10:42
@github-actions GitHub Actions
Copy link

Metric BCF-2887-context-propagation main
Coverage 73.7% 73.7%

makramkd
makramkd reviewed Oct 10, 2024
View reviewed changes
execute/tokendata/usdc/http_test.go
Comment on lines -67 to +68
getTs func() *httptest.Server
getTs func(t *testing.T) *httptest.Server
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change? I don't see the testing.T used anywhere in these funcs.

Copy link
Collaborator Author

@jmank88 jmank88 Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The default timeout case (and a few others) use a require.NoError

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see, they use the top level t *testing.T in the test func. I usually rename nested testing.T to tt but thats just preference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm I think using a different name could be error prone, because the original t is still in scope to be referenced accidentally. This happens with ctx and err often as well.

go.mod
@@ -5,8 +5,8 @@ go 1.22.5
require (
github.com/deckarep/golang-set/v2 v2.6.0
github.com/smartcontractkit/chain-selectors v1.0.23
github.com/smartcontractkit/chainlink-common v0.2.3-0.20241008175210-167715aa8613
github.com/smartcontractkit/libocr v0.0.0-20240717100443-f6226e09bee7
github.com/smartcontractkit/chainlink-common v0.3.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noice, semver!

@makramkd
Copy link
Collaborator

Integration test will fail until smartcontractkit/chainlink#14011 is merged

@jmank88
Copy link
Collaborator Author

jmank88 commented Oct 10, 2024

Integration test will fail until smartcontractkit/chainlink#14011 is merged

Is there a way to configure it to run against latest core like we do in the non-evm repos?
image

@jmank88 jmank88 merged commit ae3e8f4 into main Oct 10, 2024
3 of 4 checks passed
@jmank88 jmank88 deleted the BCF-2887-context-propagation branch October 10, 2024 12:07
winder pushed a commit that referenced this pull request Oct 10, 2024
@jmank88 @winder
bump libocr; add context (#75)
2f9c487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimkouv dimkouv dimkouv approved these changes

@makramkd makramkd makramkd approved these changes

@rstout rstout Awaiting requested review from rstout rstout is a code owner automatically assigned from smartcontractkit/ccip-offchain

@asoliman92 asoliman92 Awaiting requested review from asoliman92 asoliman92 is a code owner automatically assigned from smartcontractkit/ccip-offchain

@mateusz-sekara mateusz-sekara Awaiting requested review from mateusz-sekara mateusz-sekara is a code owner automatically assigned from smartcontractkit/ccip-offchain

@0xnogo 0xnogo Awaiting requested review from 0xnogo 0xnogo is a code owner automatically assigned from smartcontractkit/ccip-offchain

@winder winder Awaiting requested review from winder

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmank88 @makramkd @winder @dimkouv