-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
makramkd
commented
Jul 31, 2024
•
edited
Loading
edited
- 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
There was a problem hiding this 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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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 | ||
}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
)?
There was a problem hiding this comment.
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
.
Co-authored-by: Will Winder <[email protected]>
Test Coverage
|