[BCF-3392] - ContractReaderByIDs Wrapper #797

ilija42 · 2024-09-24T14:15:00Z

Adding a wrapper for this feature instead of making interface changes since not all use cases might want this

Main concern is double versioning since ContractReader changes might break this interface so we would have to version both CR and this wrapper. Right now there are no further planned changes to the interface, so at least we should be safe until new chain family impl. starts.
Had to change the fakeChainReader tester to support multiple addresses for same contract

pkg/chainreader/contract_reader.go

winder · 2024-09-26T17:54:31Z

pkg/chainreader/contract_reader.go

+type contractReaderByIDs struct {
+	bindings sync.Map
+	cr       types.ContractReader
+}


Does this need to embed UnimplementedContractReader?

Not really, UnimplementedContractReader is mainly for relayer impl. to have a stub so that the build doesn't break. If there is an interface change to Contract Reader, chainlink-common won't be mergeable before contractReaderByIDs calls to ContractReader are updated.
There is a separate concern here though, if the interface changes are breaking then the wrapper methods will have to be modified too, which means that we would have to version both the ContractReader interface and this wrapper.

There are no plans for making any changes to the interface so there is no chance of this happening for now, but this could theoretically happen when work on a new chain family starts

winder · 2024-09-26T17:56:06Z

pkg/chainreader/contract_reader.go

+		crByIds.bindings.Delete(customContractID)
+		toUnbind = append(toUnbind, boundContract)
+	}
+	return crByIds.cr.Unbind(ctx, toUnbind)


If there's an error, could crByIds.bindings be in a bad state?

I think with the current EVM impl. even in the worst scenario (LP having a dangling filter tracking a contract for no reason) this would be fine. I changed it though to future proof a bit more, we should make Unbind completely transactional so that the behaviour is easier to predict and handle when we get to multiple chain impl.

pkg/chainreader/contract_reader.go

…erByIDs-wrapper # Conflicts: # pkg/loop/internal/relayer/pluginprovider/contractreader/contract_reader_test.go

EasterTheBunny

LGTM

patrickhuie19 · 2024-09-27T17:52:50Z

pkg/chainreader/contract_reader_test.go

+	}
+
+	// Test Unbind with error shouldn't remove bindings
+	require.Error(t, crByIDs.Unbind(ctx, map[string]types.BoundContract{bc1CustomID: bc1}))


/nit is this slightly clearer for the reader if the map is empty?

Suggested change

require.Error(t, crByIDs.Unbind(ctx, map[string]types.BoundContract{bc1CustomID: bc1}))

require.Error(t, crByIDs.Unbind(ctx, map[string]types.BoundContract{}))

Ah, no, I think it's clearer as is given the Test successful Unbind case

patrickhuie19 · 2024-09-27T18:04:39Z

I don't know if I fully grok the reason for the feature request, but the change looks good 👍

* WIP * Update ContractReaderByIDs interface method names * Unexpose types.ContractReader in contractReaderByIDs * Add multiple contract address support to fakeContractReader for tests * Add GetLatestValue unit test for contractReaderByIDs * Add GetLatestValue unit test for QueryKey * Add BatchGetLatestValues unit test for CR by custom IdDs wrapper * Rm ContractReaderByIDs interface and export the struct * Change ContractReaderByIDs wrapper Unbind handling * Improve ContractReaderByIDs wrapper err handling * Remove mockery usage from ContractReaderByIDs tests * lint

* [CM-380] Identical Aggregator * [CAPPL-60] Dynamic encoder selection in OCR consensus aggregator * extract encoder name and config * Add more tests * add limit to seq num range (#781) * [chore] Handle aliases in slices (#784) * [chore] Handle aliases in slices * More aliasing tests * Lint fix * Fix test --------- Co-authored-by: Sri Kidambi <1702865+kidambisrinivas@users.noreply.github.com> * feat(observability-lib): legendoptions + improvement on node general dashboard (#785) * [CAPPL-58] Correctly stub out clock_time_get and poll_oneoff (#778) * [CAPPL-58] Further cleanup * [CAPPL-58] Add support for compression * More alias handling in Unwrap functionality of Value (#792) * Generic case to handle both pointer type and raw type and simplify int unwrap * Handling interface and default * Small test fix --------- Co-authored-by: Cedric Cordenier <cedric.cordenier@smartcontract.com> * Fix alias typing and tests (#788) * Fix alias typing and tests * Fix ints * errors.new instead of fmt * Add array support to slice (#789) * Replace fmt.Errorf with errors.New where possible (#795) * chore(workflows): adds unit test to utils (#782) * Have the mock runner register with capabilites (#783) * Add binary + config to custom compute (#794) * Add binary + config to custom compute * Add binary + config to custom compute * fix lint issues (#786) * execution factory constructor updated to take two providers, chainIDs, and source token address (#641) * execution factory constructor updated to take two providers and chain IDs (cherry picked from commit 6ad1f08) * Adding source token address to execution factory constructor * Support passing in a values.Value to the chainreader GetLatestValue method (#779) * add support for passing in a values.Value type to the contract readers GetLatestValue and QueryKey methods --------- Co-authored-by: Sri Kidambi <1702865+kidambisrinivas@users.noreply.github.com> Co-authored-by: Cedric Cordenier <cedric.cordenier@smartcontract.com> * [CAPPL-31] feat(values): adds support for time.Time as value (#787) * feat(values): adds support for time.Time as value * chore(deps): updates .tool-versions * refactor(values): uses primitive type in protos * feat(values): support float64 values (#799) * confidence level from string (#802) * Float32/Float64 wrapping (#804) * feat: implement sdk logger (#762) * Add MustEmbed Constraint to Contract Reader (#801) Reintroducing the must embed constraint to `ContractReader` implementations to ensure that all implementations of `ContractReader` embed the `UnimplementedContractReader`. If an implementation contains the unemplemented struct, changes to the interface will flow down to all implementations without introducing breaking changes. * Updated TestStruct to enable advanced querying (#798) * Updated TestStruct to enable advanced querying * linting fixes * Update pkg/codec/encodings/type_codec_test.go Co-authored-by: Clement <clement.erena78@gmail.com> * Update pkg/codec/encodings/type_codec_test.go Co-authored-by: Clement <clement.erena78@gmail.com> * Fixed codec tests --------- Co-authored-by: Clement <clement.erena78@gmail.com> * Properly support the range of uint64 and allow big int to unwrap into smaller integer types (#810) * Extract expirable cache abstraction for reuse (#807) * expirable_cache * remove cache (#812) * CCIP-3555 Attestation encoder interfaces (#813) * Attestation encoder interfaces * Attestation encoder interfaces * Attestation encoder interfaces * Comment * [BCF-3392] - ContractReaderByIDs Wrapper (#797) * WIP * Update ContractReaderByIDs interface method names * Unexpose types.ContractReader in contractReaderByIDs * Add multiple contract address support to fakeContractReader for tests * Add GetLatestValue unit test for contractReaderByIDs * Add GetLatestValue unit test for QueryKey * Add BatchGetLatestValues unit test for CR by custom IdDs wrapper * Rm ContractReaderByIDs interface and export the struct * Change ContractReaderByIDs wrapper Unbind handling * Improve ContractReaderByIDs wrapper err handling * Remove mockery usage from ContractReaderByIDs tests * lint * pkg/types/ccipocr3: add DestExecData to RampTokenAmount (#817) * pkg/types/ccipocr3: add DestExecData to RampTokenAmount * fix test * Allow the creation of maps from string to capbility outputs. (#815) * Add the FeeValueJuels field to ccipocr3.Message (#819) * feat(observability-lib): improve alerts rule (#803) * feat(observability-lib): improve alerts rule * chore(observability-lib): README + folder structure (#806) * chore(observability-lib): README + folder structure * feat(observability-lib): variable add current + includeAll options (#808) * chore(README): small corrections * chore(README): example improved * chore(README): add references to dashboards examples * feat(observability-lib): refactor exportable func + link to godoc * fix(observability-lib): cmd errors returns * enable errorf check (#826) * Make overridding the encoder first-class * Update mocks * Mock updates * Adjust tests * Fix mock * Fix mock * Update mock * Linting --------- Co-authored-by: Cedric Cordenier <cedric.cordenier@smartcontract.com> Co-authored-by: dimitris <dimitrios.kouveris@smartcontract.com> Co-authored-by: Sri Kidambi <1702865+kidambisrinivas@users.noreply.github.com> Co-authored-by: Clement <clement.erena78@gmail.com> Co-authored-by: Ryan Tinianov <tinianov@live.com> Co-authored-by: Street <5597260+MStreet3@users.noreply.github.com> Co-authored-by: Jordan Krage <jmank88@gmail.com> Co-authored-by: Patrick <patrick.huie@smartcontract.com> Co-authored-by: Matthew Pendrey <matthew.pendrey@gmail.com> Co-authored-by: Gabriel Paradiso <gaboparadiso@gmail.com> Co-authored-by: Awbrey Hughlett <athughlett@gmail.com> Co-authored-by: Silas Lenihan <32529249+silaslenihan@users.noreply.github.com> Co-authored-by: Mateusz Sekara <mateusz.sekara@gmail.com> Co-authored-by: ilija42 <57732589+ilija42@users.noreply.github.com> Co-authored-by: Makram <makramkd@users.noreply.github.com> Co-authored-by: Ryan Stout <rstout610@gmail.com>

WIP

9d835bf

ilija42 temporarily deployed to integration September 24, 2024 14:15 — with GitHub Actions Inactive

ilija42 requested a review from pablolagreca September 24, 2024 15:06

Update ContractReaderByIDs interface method names

1d52870

ilija42 had a problem deploying to integration September 24, 2024 15:13 — with GitHub Actions Error

ilija42 force-pushed the BCF-3392-ContractReaderByIDs-wrapper branch from 522b0bc to f71c25a Compare September 24, 2024 15:14

ilija42 temporarily deployed to integration September 24, 2024 15:14 — with GitHub Actions Inactive

ilija42 had a problem deploying to integration September 24, 2024 15:14 — with GitHub Actions Error

Unexpose types.ContractReader in contractReaderByIDs

1ff70a2

ilija42 force-pushed the BCF-3392-ContractReaderByIDs-wrapper branch from f71c25a to 1ff70a2 Compare September 24, 2024 15:18

ilija42 temporarily deployed to integration September 24, 2024 15:18 — with GitHub Actions Inactive

ilija42 added 3 commits September 26, 2024 12:56

Add multiple contract address support to fakeContractReader for tests

2185ec8

Add GetLatestValue unit test for contractReaderByIDs

e2f44c7

Add GetLatestValue unit test for QueryKey

3e3b1ff

winder reviewed Sep 26, 2024

View reviewed changes

pkg/chainreader/contract_reader.go Outdated Show resolved Hide resolved

winder reviewed Sep 26, 2024

View reviewed changes

pkg/chainreader/contract_reader.go Outdated Show resolved Hide resolved

ilija42 added 3 commits September 27, 2024 13:14

Add BatchGetLatestValues unit test for CR by custom IdDs wrapper

91c45d4

Rm ContractReaderByIDs interface and export the struct

ec777a6

Change ContractReaderByIDs wrapper Unbind handling

fd5c319

ilija42 changed the title ~~WIP~~ [BCF-3392] - ContractReaderByIDs Wrapper Sep 27, 2024

Improve ContractReaderByIDs wrapper err handling

dfd985f

ilija42 added 2 commits September 27, 2024 15:23

Merge remote-tracking branch 'origin/main' into BCF-3392-ContractRead…

9d0b26b

…erByIDs-wrapper # Conflicts: # pkg/loop/internal/relayer/pluginprovider/contractreader/contract_reader_test.go

Remove mockery usage from ContractReaderByIDs tests

16424c6

ilija42 temporarily deployed to integration September 27, 2024 13:53 — with GitHub Actions Inactive

ilija42 had a problem deploying to integration September 27, 2024 13:53 — with GitHub Actions Failure

ilija42 temporarily deployed to integration September 27, 2024 13:53 — with GitHub Actions Inactive

ilija42 marked this pull request as ready for review September 27, 2024 13:54

ilija42 requested a review from a team as a code owner September 27, 2024 13:54

ilija42 requested review from patrickhuie19 and removed request for a team September 27, 2024 13:54

ilija42 had a problem deploying to integration September 27, 2024 14:00 — with GitHub Actions Failure

lint

7376e82

ilija42 temporarily deployed to integration September 27, 2024 14:09 — with GitHub Actions Inactive

ilija42 had a problem deploying to integration September 27, 2024 14:09 — with GitHub Actions Failure

ilija42 requested a review from winder September 27, 2024 14:11

EasterTheBunny approved these changes Sep 27, 2024

View reviewed changes

patrickhuie19 reviewed Sep 27, 2024

View reviewed changes

patrickhuie19 approved these changes Sep 27, 2024

View reviewed changes

Farber98 approved these changes Sep 27, 2024

View reviewed changes

winder approved these changes Sep 30, 2024

View reviewed changes

Merge branch 'main' into BCF-3392-ContractReaderByIDs-wrapper

84714e0

ilija42 temporarily deployed to integration September 30, 2024 16:49 — with GitHub Actions Inactive

ilija42 merged commit f10ba2b into main Sep 30, 2024
11 checks passed

ilija42 deleted the BCF-3392-ContractReaderByIDs-wrapper branch September 30, 2024 17:03

samsondav mentioned this pull request Oct 8, 2024

Implement RetirementReport format #835

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BCF-3392] - ContractReaderByIDs Wrapper #797

[BCF-3392] - ContractReaderByIDs Wrapper #797

ilija42 commented Sep 24, 2024 •

edited

Loading

winder Sep 26, 2024

ilija42 Sep 27, 2024 •

edited

Loading

ilija42 Sep 27, 2024 •

edited

Loading

winder Sep 26, 2024 •

edited

Loading

ilija42 Sep 27, 2024

EasterTheBunny left a comment

patrickhuie19 Sep 27, 2024

patrickhuie19 Sep 27, 2024

patrickhuie19 commented Sep 27, 2024

	require.Error(t, crByIDs.Unbind(ctx, map[string]types.BoundContract{bc1CustomID: bc1}))
	require.Error(t, crByIDs.Unbind(ctx, map[string]types.BoundContract{}))

[BCF-3392] - ContractReaderByIDs Wrapper #797

[BCF-3392] - ContractReaderByIDs Wrapper #797

Conversation

ilija42 commented Sep 24, 2024 • edited Loading

Adding a wrapper for this feature instead of making interface changes since not all use cases might want this

winder Sep 26, 2024

Choose a reason for hiding this comment

ilija42 Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

ilija42 Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

winder Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

ilija42 Sep 27, 2024

Choose a reason for hiding this comment

EasterTheBunny left a comment

Choose a reason for hiding this comment

patrickhuie19 Sep 27, 2024

Choose a reason for hiding this comment

patrickhuie19 Sep 27, 2024

Choose a reason for hiding this comment

patrickhuie19 commented Sep 27, 2024

ilija42 commented Sep 24, 2024 •

edited

Loading

ilija42 Sep 27, 2024 •

edited

Loading

ilija42 Sep 27, 2024 •

edited

Loading

winder Sep 26, 2024 •

edited

Loading