Implement CommitPlugin with RMN OffChain Blessing #17
Conversation
rstout
force-pushed
the
rs/commit_plugin_rmn_ocb
branch
6 times, most recently
from
f3e3035 to
113e94e
Compare
rstout
force-pushed
the
rs/commit_plugin_rmn_ocb
branch
11 times, most recently
from
d62f0b3 to
1608eb2
Compare
rstout
force-pushed
the
rs/commit_plugin_rmn_ocb
branch
from
f22aa47 to
e26270d
Compare
commit_rmn_ocb/types.go
Outdated
| // GetOnRampMaxSeqNums returns the maximum sequence number that this oracle finds on the OnRamp for each given chain | ||
| // (for the configured dest chain) | ||
| GetOnRampMaxSeqNums() ([]plugintypes.SeqNumChain, error) |
There was a problem hiding this comment.
I think we should be a bit more precise here in terms of what we want from this method and try to match existing terminology in the ramp contracts so we don't get confused.
The onramp has this method: getExpectedNextSequenceNumber which returns the next sequence number that is going to be used by the onramp. Is this what we want?
Or do we want the last-used sequence number, which would be getExpectedNextSequenceNumber() - 1?
There was a problem hiding this comment.
getExpectedNextSequenceNumber is a method on the OffRamp, not the OnRamp right? I've renamed the OffRamp seq nums to OffRampNextSeqNums and kept OnRampMaxSeqNums, we can discuss changing the names further.
commit_rmn_ocb/types.go
Outdated
| // GetOffRampMaxSeqNums returns the maximum sequence number that this oracle finds on the OffRamp for each given | ||
| // chain (for the configured dest chain) | ||
| GetOffRampMaxSeqNums() ([]plugintypes.SeqNumChain, error) |
There was a problem hiding this comment.
Another piece of terminology / consistency in order to avoid confusion: these have been called "max committed sequence numbers" so maybe call this GetMaxCommittedSeqNums
There was a problem hiding this comment.
I've renamed this to OffRampNextSeqNums, but happy to discuss clearer names
commit_rmn_ocb/types.go
Outdated
| case ReportIntervalsSelected: | ||
| return BuildingReport | ||
| case ReportGenerated: | ||
| return WaitingForReportTransmission | ||
| case ReportEmpty: | ||
| return SelectingRangesForReport | ||
| case ReportNotYetTransmitted: | ||
| return WaitingForReportTransmission | ||
| case ReportTransmitted: | ||
| return SelectingRangesForReport | ||
| case ReportNotTransmitted: | ||
| return SelectingRangesForReport |
There was a problem hiding this comment.
nit: This would be more readable if the transitions that lead to the same state are grouped together, e.g
switch o.OutcomeType {
case ReportIntervalsSelected:
return BuildingReport
case ReportGenerated:
fallthrough
case ReportNotYetTransmitted:
return WaitingForReportTransmission
case ReportEmpty:
fallthrough
case ReportTransmitted:
fallthrough
case ReportNotTransmitted:
fallthrough
default:
return SelectingRangesForReport
}Feels like this might be better as a pure func rather than tied to the Outcome, but I'm not too opinionated about that. Also needs a set of tests to cover all the transitions.
|
|
||
| func validateFChain(fChain map[cciptypes.ChainSelector]int) error { | ||
| for _, f := range fChain { | ||
| if f < 0 { |
There was a problem hiding this comment.
Should be f <= 0 since f is never 0.
There was a problem hiding this comment.
f can be 0 for chains that are not RMN enabled
commit_rmn_ocb/plugin.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (p *Plugin) decodeOutcome(outcome ocr3types.Outcome) (CommitPluginOutcome, CommitPluginState) { |
There was a problem hiding this comment.
Feels like this should return an error instead of just returning SelectingRangesForReport if there is an error decoding
There was a problem hiding this comment.
My logic is: if there's an issue decoding, just go to the "select interval" state instead of erroring-out
rstout
force-pushed
the
rs/commit_plugin_rmn_ocb
branch
6 times, most recently
from
f37494d to
e245649
Compare
rstout
force-pushed
the
rs/commit_plugin_rmn_ocb
branch
4 times, most recently
from
af41868 to
4506323
Compare
|
Looks good overall, mostly minor comments above 😄 |
rstout
force-pushed
the
rs/commit_plugin_rmn_ocb
branch
from
0f8eced to
ffb55ad
Compare
There was a problem hiding this comment.
Mostly nits and questions. Looks great overall.
I did have some confusion around the transient nature of states and the outcome type -- especially in the Reports function where it would have been nice to check for a BuildingReport state rather than a ReportGenerated outcome.
|
|
||
| const ( | ||
| ReportIntervalsSelected OutcomeType = iota + 1 | ||
| ReportGenerated |
There was a problem hiding this comment.
nit: the report doesn't generate until after the outcome. I'd also consider removing the Report prefix from all of these.
| ReportGenerated | |
| ReportGeneration |
There was a problem hiding this comment.
I prefer past tense, as in practice these are the states of the previous outcome. I think dropping "Report" might lose some clarity imo.
rstout
force-pushed
the
rs/commit_plugin_rmn_ocb
branch
from
ffb55ad to
d259ddc
Compare
rstout
force-pushed
the
rs/commit_plugin_rmn_ocb
branch
from
d259ddc to
3afd871
Compare
Test Coverage
|
Implements a new version of the Commit Plugin in /commit_rmn_ocb (ocb = off chain blessing) based on the spec. This is basically only the OCR3 function implementations (e.g. Query, Observation, Outcome, etc). No tests have been written yet. I recommend starting with the README.