Remove error on missing value in destTokenPricesUSD #692
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I see you updated files related to core. Please run |
RensR
commented
Apr 8, 2024
| @@ -256,18 +269,13 @@ func (r *ExecutionReportingPlugin) buildBatch( | |||
| ctx context.Context, | |||
| lggr logger.Logger, | |||
| report commitReportWithSendRequests, | |||
| inflight []InflightInternalExecutionReport, | |||
| inflightAggregateValue *big.Int, | |||
There was a problem hiding this comment.
Only used once, extracted the calculation to the function above to keep this cleaner and easier to test
RensR
commented
Apr 8, 2024
| @@ -527,7 +535,8 @@ func aggregateTokenValue(destTokenPricesUSD map[cciptypes.Address]*big.Int, sour | |||
| for i := 0; i < len(tokensAndAmount); i++ { | |||
| price, ok := destTokenPricesUSD[sourceToDest[tokensAndAmount[i].Token]] | |||
| if !ok { | |||
| return nil, errors.Errorf("do not have price for source token %v", tokensAndAmount[i].Token) | |||
| // If we don't have a price for the token, we will assume it's worth 0. | |||
There was a problem hiding this comment.
Main change of this PR
There was a problem hiding this comment.
Should we log these 0 token prices for the debug purpose?
RensR
commented
Apr 8, 2024
| @@ -775,25 +784,6 @@ func (r *ExecutionReportingPlugin) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func inflightAggregates( | |||
There was a problem hiding this comment.
Since the removal of most of the logic of this func in the previous PR, we can inline this function
There was a problem hiding this comment.
I would keep it extracted as it was. It's easier for me to read through the code when this behaviour is extracted than inlined
RensR
commented
Apr 8, 2024
| @@ -254,10 +254,6 @@ func (o *OffRamp) CurrentRateLimiterState(ctx context.Context) (cciptypes.TokenB | |||
| }, nil | |||
| } | |||
|
|
|||
| func (o *OffRamp) GetDestinationToken(ctx context.Context, address common.Address) (common.Address, error) { | |||
mateusz-sekara
approved these changes
Apr 8, 2024
asoliman92
pushed a commit
that referenced
this pull request
Jul 31, 2024
## Motivation Self serve will not offer a full mapping of source to dest tokens. This PR enables optimistic aggregate rate limiting, meaning it will only calculate the rate limit value for tokens which are mapped. For all lanes before 1.5, this means no change. For the 1.5+ lanes it means no token will be included. This will be fixed in a future PR, before 1.5 will ever be deployed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Self serve will not offer a full mapping of source to dest tokens. This PR enables optimistic aggregate rate limiting, meaning it will only calculate the rate limit value for tokens which are mapped. For all lanes before 1.5, this means no change. For the 1.5+ lanes it means no token will be included. This will be fixed in a future PR, before 1.5 will ever be deployed.