exec: Skip messages that do not fit #40
Conversation
| builder := NewBuilder(ctx, lggr, hasher, tokenDataReader, codec, tt.args.maxReportSize, 0) | ||
| var updatedMessages []plugintypes.ExecutePluginCommitDataWithMessages | ||
| for _, report := range tt.args.reports { | ||
| updatedMessage, err := builder.Add(report) | ||
| if err != nil && tt.wantErr != "" { | ||
| if strings.Contains(err.Error(), tt.wantErr) { | ||
| foundError = true | ||
| break | ||
| } | ||
| } | ||
| require.NoError(t, err) | ||
| updatedMessages = append(updatedMessages, updatedMessage) | ||
| } |
There was a problem hiding this comment.
Almost identical to the previous selectReports test. This part was swapped to use the builder directly, and there are 2 new test cases to cover the "skip over message that doesn't fit" logic.
| { | ||
| name: "bad codec", | ||
| wantErr: "unable to encode report: bad codec", | ||
| args: args{ | ||
| report: plugintypes.ExecutePluginCommitDataWithMessages{ | ||
| ExecutePluginCommitData: plugintypes.ExecutePluginCommitData{ | ||
| SourceChain: 1234567, | ||
| SequenceNumberRange: cciptypes.NewSeqNumRange(cciptypes.SeqNum(100), cciptypes.SeqNum(100)), | ||
| }, | ||
| Messages: []cciptypes.Message{ | ||
| {Header: cciptypes.RampMessageHeader{ | ||
| SourceChainSelector: 1234567, | ||
| SequenceNumber: cciptypes.SeqNum(100), | ||
| }}, | ||
| }, | ||
| }, | ||
| codec: badCodec{}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This case will return when I test the new checkMessage and verifyReport methods.
| return false | ||
| finalReport = cciptypes.ExecutePluginReportSingleChain{} | ||
| msgs := make(map[int]struct{}) | ||
| for i := range report.Messages { |
There was a problem hiding this comment.
@asoliman92 this is where sort.Search is replaced. The algorithm now adds messages in one at a time rather than attempting a binary search. I'm still not entirely happy with either approach.
| return ReadyToExecute, nil | ||
| } | ||
|
|
||
| func (b *execReportBuilder) verifyReport( |
There was a problem hiding this comment.
Notice validationMetadata, this type holds the encoded size and will also hold the gas limit and perhaps the fee boost ratio. It is computed per report, and accumulated as part of the builder.
| type messageStatus string | ||
|
|
||
| const ( | ||
| ReadyToExecute messageStatus = "ready_to_execute" | ||
| AlreadyExecuted messageStatus = "already_executed" | ||
| /* | ||
| SenderAlreadySkipped messageStatus = "sender_already_skipped" | ||
| MessageMaxGasCalcError messageStatus = "message_max_gas_calc_error" | ||
| InsufficientRemainingBatchDataLength messageStatus = "insufficient_remaining_batch_data_length" | ||
| InsufficientRemainingBatchGas messageStatus = "insufficient_remaining_batch_gas" | ||
| MissingNonce messageStatus = "missing_nonce" | ||
| InvalidNonce messageStatus = "invalid_nonce" | ||
| AggregateTokenValueComputeError messageStatus = "aggregate_token_value_compute_error" | ||
| AggregateTokenLimitExceeded messageStatus = "aggregate_token_limit_exceeded" | ||
| TokenDataNotReady messageStatus = "token_data_not_ready" | ||
| TokenDataFetchError messageStatus = "token_data_fetch_error" | ||
| TokenNotInDestTokenPrices messageStatus = "token_not_in_dest_token_prices" | ||
| TokenNotInSrcTokenPrices messageStatus = "token_not_in_src_token_prices" | ||
| InsufficientRemainingFee messageStatus = "insufficient_remaining_fee" | ||
| AddedToBatch messageStatus = "added_to_batch" | ||
| */ | ||
| ) |
There was a problem hiding this comment.
These are all the things in the ocr2 implementation. I guess that all of them need to be part of the new verify/check methods.
| ) (cciptypes.ExecutePluginReportSingleChain, int, error) { | ||
| if maxMessages == 0 { | ||
| maxMessages = len(report.Messages) | ||
| messages map[int]struct{}, |
There was a problem hiding this comment.
Switched from taking the first N messages, to taking specific messages. Manipulating the content of this map is how messages are skipped.
| buildSingleChainReportMaxSize(b.ctx, b.lggr, b.hasher, b.tokenDataReader, b.encoder, | ||
| commitReport, | ||
| int(b.maxReportSizeBytes-b.reportSizeBytes)) | ||
| execReport, updatedReport, err := b.buildSingleChainReport(b.ctx, commitReport) | ||
|
|
There was a problem hiding this comment.
Notice that the maximum size is gone, it's now part of the object along with the provider arguments.
winder
force-pushed
the
will/skip-messages-that-do-not-fit
branch
from
12e237c
to
6a556e5
Compare
winder
force-pushed
the
will/skip-messages-that-do-not-fit
branch
2 times, most recently
from
f258998
to
23a10bf
Compare
winder
force-pushed
the
will/skip-messages-that-do-not-fit
branch
from
23a10bf
to
4e7ed29
Compare
Test Coverage
|
Three main changes:
selectReporttest to use the report builder, move to report builder package.buildSingleChainReportto select reports by index rather than by a maximum number of messages to select.checkMesssageandverifyReportfunctions to contain business logic to determine whether messages are ready to be executed and if the final report is valid.