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.

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

Use extended contract reader, set onramp correctly in cciptypes.Message #52

Merged
merged 13 commits into from
Aug 1, 2024

Conversation

makramkd
Copy link
Collaborator

@makramkd makramkd commented Jul 31, 2024

  • fix the CCIP reader to use the extended contract reader and set the onramp address correctly on CCIP messages
  • fix outcome encoding to be deterministic

execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/report/report.go Outdated Show resolved Hide resolved
execute/report/report.go Outdated Show resolved Hide resolved
execute/report/report.go Outdated Show resolved Hide resolved
execute/report/report.go Outdated Show resolved Hide resolved
execute/report/roots.go Outdated Show resolved Hide resolved
@makramkd makramkd changed the title Mk/exec logging Use extended contract reader, set onramp correctly in cciptypes.Message Aug 1, 2024
@makramkd makramkd marked this pull request as ready for review August 1, 2024 09:42
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin.go Outdated Show resolved Hide resolved
execute/plugin_e2e_test.go Outdated Show resolved Hide resolved
internal/reader/ccip.go Outdated Show resolved Hide resolved
internal/reader/ccip.go Outdated Show resolved Hide resolved
winder
winder previously requested changes Aug 1, 2024
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple small suggestions

@@ -345,6 +369,10 @@ func (p *Plugin) Outcome(
return commitReports[i].Timestamp.Before(commitReports[j].Timestamp)
})

p.lggr.Debugw(
fmt.Sprintf("[oracle %d] exec outcome: commit reports", p.reportingCfg.OracleID),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use sprintf instead of a key/value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having the oracle ID in the log message is much quicker to parse than having it in the fields; so I want a formatted string as the log message but still want to JSON encode the log fields.

}
_, err := p.ShouldAcceptAttestedReport(context.Background(), 0, ocr3types.ReportWithInfo[[]byte]{})
_, err := p.ShouldAcceptAttestedReport(context.Background(), 0, ocr3types.ReportWithInfo[[]byte]{
Report: []byte("will not decode"), // faked out, see mock above
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this parameter needed or informational? I thought the mocked function would be enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its needed since I updated the Reports function to return if there is no report, since that will cause JSON decoding to error out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And it gives a more informative log in that scenario

execute/report/report.go Outdated Show resolved Hide resolved
Comment on lines +97 to +108
pendingCommitsCP := append([]ExecutePluginCommitData{}, pendingCommits...)
reportCP := append([]cciptypes.ExecutePluginReportSingleChain{}, report.ChainReports...)
sort.Slice(
pendingCommitsCP,
func(i, j int) bool {
return pendingCommitsCP[i].SourceChain < pendingCommitsCP[j].SourceChain
})
sort.Slice(
reportCP,
func(i, j int) bool {
return reportCP[i].SourceChainSelector < reportCP[j].SourceChainSelector
})
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO business logic shouldn't be here, move the sorting to plugin.go

Also, should the sequence number be considered when sorting? I forget which data structures are only supposed to have one object per source chain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if this is business logic but encoding seems to have been non-deterministic before, hence this sorting change.

We might want to always ensure this sortation in the Encode method rather than depend on the caller / constructor to make sure its sorted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

re: sequence number considerations while sorting, are you referring to the case where the same source chain selector can have many different commit reports (in []ExecutePluginCommitData)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed offline, duplicated sorting logic in Encode.

Copy link

github-actions bot commented Aug 1, 2024

Test Coverage

Branch Coverage
mk/exec-logging 76.5%
ccip-develop 77.4%

@makramkd makramkd merged commit 456a6df into ccip-develop Aug 1, 2024
2 checks passed
@makramkd makramkd deleted the mk/exec-logging branch August 1, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants