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

event loader with event watch and backfill #911

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

event loader with event watch and backfill #911

wants to merge 16 commits into from
+6,501 −1,846

Conversation

EasterTheBunny
Copy link
Contributor

@EasterTheBunny EasterTheBunny commented Oct 31, 2024 •
edited by jira bot
Loading

Description

This PR introduces a component that monitors solana slot production and loads program log messages looking for emitted events. This component also provides a backfill feature to look for events in past blocks by program address.

Included is an integration test to a local solana validator as a smoke test and a benchmark test for watching slot production.

goos: darwin
goarch: arm64
pkg: github.com/smartcontractkit/chainlink-solana/pkg/solana/logpoller
cpu: Apple M1 Pro
BenchmarkEncodedLogCollector-10          1770250              6545 ns/op              6910 events/sec            2.762 rcp_calls/sec         662 B/op          3 allocs/op
PASS
ok      github.com/smartcontractkit/chainlink-solana/pkg/solana/logpoller       18.798s

NONEVM-742

@EasterTheBunny EasterTheBunny requested review from a team as code owners October 31, 2024 13:40
@EasterTheBunny EasterTheBunny requested a review from a team as a code owner October 31, 2024 14:24
reductionista
reductionista reviewed Nov 2, 2024
View reviewed changes
pkg/solana/logpoller/parser.go
instLogs[len(instLogs)-1].Failed = true

idx := strings.Index(log, ": ") + 2
currText := fmt.Sprintf(`Program returned error: "%s"`, log[idx:])
Copy link
Contributor

@reductionista reductionista Nov 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting... this looks useful, but I didn't think it was in scope for what we were trying to support (reading success or failure of contract calls, as opposed to just indexing events). Is this something CCIP mentioned they want, or is it just included here in case it comes in handy at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just in case it is needed. I carried it over from my initial POC and didn't feel like losing it as a feature.

reductionista
reductionista reviewed Nov 12, 2024
View reviewed changes
pkg/solana/logpoller/loader.go
@@ -249,7 +251,10 @@ func (c *EncodedLogCollector) loadSlotBlocksRange(ctx context.Context, start, en
}

for _, block := range result {
c.chBlock <- block
select {
case <-ctx.Done():
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't there be a break statement here to break out of the for loop if the context is cancelled?

Copy link
Contributor

@reductionista reductionista Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, also in this line at the beginning of the loadSlotBlockRange method where you add an rpc timeout... I think we should rename the child context to something else and use that only for the call to GetBlocks... making use of the parent context here.

ctx, cancel := context.WithTimeout(ctx, c.rpcTimeLimit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That can provide assurance that the function exits immediately. I'll add it.

Copy link
Contributor Author

@EasterTheBunny EasterTheBunny Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The linter doesn't seem to care about variable name shadowing here, probably because it the first and only reset of ctx within the function and the scope doesn't change (making the meaning of ctx consistent for all the function). I don't think in this ... context ... variable name shadowing applies.

reductionista
reductionista reviewed Nov 12, 2024
View reviewed changes
pkg/solana/logpoller/loader.go Outdated
if err := c.loadSlotBlocksRange(ctx, start, end); err != nil {
// a retry will happen anyway on the next round of slots
// so the error is handled by doing nothing
c.lggr.Error("failed to load slot blocks range", "start", start, "end", end, "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be c.lggr.Errorw to work

@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
47.6% Coverage on New Code (required ≥ 75%)

See analysis details on SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhaidashenko dhaidashenko dhaidashenko left review comments

@reductionista reductionista reductionista left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@EasterTheBunny @reductionista @dhaidashenko