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

[BCF-3269] - Contract Reader Batch Call #13635

Merged
merged 17 commits into from
Jul 12, 2024
Merged

[BCF-3269] - Contract Reader Batch Call #13635

merged 17 commits into from
Jul 12, 2024

Conversation

ilija42
Copy link
Contributor

@ilija42 ilija42 commented Jun 20, 2024 •
edited
Loading

Ticket

Requires Dependencies

Common
Solana

@ilija42 ilija42 changed the title Chain Reader Batch Call inital commit Chain Reader Batch Call Jun 23, 2024
@ilija42 ilija42 force-pushed the BCF-3269-batch-getlatestvalue branch from 2858a55 to fd16e08 Compare June 25, 2024 17:59
@ilija42 ilija42 force-pushed the BCF-3269-batch-getlatestvalue branch from fd16e08 to 09d1533 Compare June 26, 2024 10:11
@ilija42 ilija42 force-pushed the BCF-3269-batch-getlatestvalue branch from 09d1533 to 742d79a Compare June 26, 2024 14:08
@ilija42 ilija42 force-pushed the BCF-3269-batch-getlatestvalue branch from e534f6e to eafb642 Compare June 26, 2024 14:23
@ilija42 ilija42 mentioned this pull request Jun 26, 2024
Merged
@ilija42 ilija42 changed the title Chain Reader Batch Call [BCF-3269] - Chain Reader Batch Call Jun 26, 2024
@ilija42 ilija42 force-pushed the BCF-3269-batch-getlatestvalue branch from fbed94f to 03da86b Compare June 27, 2024 12:45
ilija42
ilija42 commented Jun 27, 2024
View reviewed changes
core/services/relay/evm/batch_caller.go
return c.bc.batchCallDynamicLimitRetries(ctx, blockNumber, reqs)
}

type defaultEvmBatchCaller struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

batchCaller and its tests are identical to what CCIP is currently using, with some adjustments to make it work better with Chain Reader interface

Copy link
Contributor

Choose a reason for hiding this comment

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

This parallel batch caller is something now used by at least ChainReader, CCIP, Automation, and Atlas with various minor differences. We should make a common component to do this with various post-processing hooks for the batch results. Then we wouldn't have so much code duplication.

Copy link
Contributor Author

@ilija42 ilija42 Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that the work to do this is way greater than maintaining two batchCallers since batchCaller isn't something that is frequently updated

@ilija42 ilija42 marked this pull request as ready for review June 27, 2024 12:58
@ilija42 ilija42 requested review from a team as code owners June 27, 2024 12:58
@ilija42 ilija42 force-pushed the BCF-3269-batch-getlatestvalue branch from 03da86b to d637399 Compare June 27, 2024 16:08
@ilija42 ilija42 mentioned this pull request Jun 27, 2024
Merged
@ilija42 ilija42 marked this pull request as draft June 27, 2024 16:49
@ilija42 ilija42 marked this pull request as ready for review June 27, 2024 17:27
@ilija42 ilija42 force-pushed the BCF-3269-batch-getlatestvalue branch from d637399 to f4815f1 Compare June 28, 2024 08:57
@ilija42 ilija42 force-pushed the BCF-3269-batch-getlatestvalue branch 2 times, most recently from 715c38e to 015c137 Compare June 28, 2024 11:18
@ilija42 ilija42 force-pushed the BCF-3269-batch-getlatestvalue branch from 2a1a846 to a843b27 Compare July 10, 2024 16:08
ilija42 added 3 commits July 12, 2024 11:23
@ilija42
Merge remote-tracking branch 'origin/develop' into BCF-3269-batch-get…
1cf07d5 
…latestvalue

# Conflicts:
#	core/scripts/go.mod
#	core/scripts/go.sum
#	core/services/relay/evm/chain_reader.go
#	core/services/relay/evm/evmtesting/chain_reader_interface_tester.go
#	go.mod
#	go.sum
#	integration-tests/go.mod
#	integration-tests/go.sum
#	integration-tests/load/go.mod
#	integration-tests/load/go.sum
@ilija42
fix minor merge issues
c67a5eb
@ilija42
Add common codec mock for batch_caller_test
14ade15
@ilija42 ilija42 requested a review from cedric-cordenier July 12, 2024 15:02
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
3 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
83.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@ilija42 ilija42 added this pull request to the merge queue Jul 12, 2024
Merged via the queue into develop with commit 055a9d2 Jul 12, 2024
118 checks passed
@ilija42 ilija42 deleted the BCF-3269-batch-getlatestvalue branch July 12, 2024 15:26
This was referenced Jul 12, 2024
Draft
Closed
@chainchad chainchad mentioned this pull request Jul 29, 2024
Merged
This was referenced Jul 31, 2024
Draft
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cedric-cordenier cedric-cordenier cedric-cordenier approved these changes

@jmank88 jmank88 jmank88 approved these changes

@EasterTheBunny EasterTheBunny Awaiting requested review from EasterTheBunny

@krehermann krehermann Awaiting requested review from krehermann

@patrickhuie19 patrickhuie19 Awaiting requested review from patrickhuie19

@dhaidashenko dhaidashenko Awaiting requested review from dhaidashenko

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
@ilija42 @jmank88 @EasterTheBunny @cedric-cordenier