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.

Sign up for GitHub

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

Do not return the error in case if onRamp not initialized #1487

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Do not return the error in case if onRamp not initialized #1487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/services/ocr2/plugins/ccip/estimatorconfig/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package estimatorconfig

import (
"context"
"errors"
"math/big"

"github.com/smartcontractkit/chainlink-common/pkg/types/ccip"
Expand Down Expand Up @@ -42,7 +41,7 @@ func (c *FeeEstimatorConfigService) SetOnRampReader(reader ccip.OnRampReader) {
// GetDynamicConfig should be cached in the onRamp reader to avoid unnecessary on-chain calls
func (c *FeeEstimatorConfigService) GetDataAvailabilityConfig(ctx context.Context) (destDataAvailabilityOverheadGas, destGasPerDataAvailabilityByte, destDataAvailabilityMultiplierBps int64, err error) {
if c.onRampReader == nil {
return 0, 0, 0, errors.New("no OnRampReader has been configured")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share some reasoning behind this decision? IIUC this will return 0s to daOverheadGas, gasPerDAByte, daMultiplier. Does the calling function know how to handle that case? What would happen to estimateDACostUSD when 0s are returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mateusz-sekara Calling this function will multiply gasPrice with 0 and return 0 DAGasPrice
https://github.com/smartcontractkit/ccip/blob/ccip-develop/core/services/ocr2/plugins/ccip/prices/da_price_estimator.go#L182

Then just added 0 gas price to the estimatedExecCost
https://github.com/smartcontractkit/ccip/blob/ccip-develop/core/services/ocr2/plugins/ccip/prices/da_price_estimator.go#L154

This is worked in the same way before this feature been implemented

Copy link
Contributor

@mateusz-sekara mateusz-sekara Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, I would consider some warning log in this if if that's something that should not happen

return 0, 0, 0, nil
}

cfg, err := c.onRampReader.GetDynamicConfig(ctx)
Expand Down
9 changes: 6 additions & 3 deletions core/services/ocr2/plugins/ccip/estimatorconfig/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@ func TestFeeEstimatorConfigService(t *testing.T) {
var expectedDestDataAvailabilityMultiplierBps int64 = 3

onRampReader := mocks.NewOnRampReader(t)
_, _, _, err := svc.GetDataAvailabilityConfig(ctx)
require.Error(t, err)
destDataAvailabilityOverheadGas, destGasPerDataAvailabilityByte, destDataAvailabilityMultiplierBps, err := svc.GetDataAvailabilityConfig(ctx)
require.NoError(t, err) // if onRampReader not set, return nil error and 0 values
require.EqualValues(t, 0, destDataAvailabilityOverheadGas)
require.EqualValues(t, 0, destGasPerDataAvailabilityByte)
require.EqualValues(t, 0, destDataAvailabilityMultiplierBps)
svc.SetOnRampReader(onRampReader)

onRampReader.On("GetDynamicConfig", ctx).
Expand All @@ -35,7 +38,7 @@ func TestFeeEstimatorConfigService(t *testing.T) {
DestDataAvailabilityMultiplierBps: uint16(expectedDestDataAvailabilityMultiplierBps),
}, nil).Once()

destDataAvailabilityOverheadGas, destGasPerDataAvailabilityByte, destDataAvailabilityMultiplierBps, err := svc.GetDataAvailabilityConfig(ctx)
destDataAvailabilityOverheadGas, destGasPerDataAvailabilityByte, destDataAvailabilityMultiplierBps, err = svc.GetDataAvailabilityConfig(ctx)
require.NoError(t, err)
require.Equal(t, expectedDestDataAvailabilityOverheadGas, destDataAvailabilityOverheadGas)
require.Equal(t, expectedDestGasPerDataAvailabilityByte, destGasPerDataAvailabilityByte)
Expand Down
Loading