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

Change PluginMedian Interface #166

Closed
wants to merge 21 commits into from
Closed

Change PluginMedian Interface #166

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f56670e
change interface
augustbleeds Sep 5, 2023
3e033ff
change signature for median factory
augustbleeds Sep 5, 2023
f8416cd
Merge branch 'main' into augustus.gas-prices-changes
augustbleeds Sep 6, 2023
c988207
serve gasPrice over grpc
augustbleeds Sep 8, 2023
c7bc6d8
update protobuf
augustbleeds Sep 8, 2023
0aa1d2d
add tests
augustbleeds Sep 8, 2023
7e621d3
Merge branch 'main' into augustus.gas-prices-changes
augustbleeds Sep 8, 2023
e51f862
lint fix
augustbleeds Sep 18, 2023
c604ab9
change to noop data source
augustbleeds Jan 12, 2024
852340c
merge main
augustbleeds Jan 12, 2024
cde3993
handle noop
augustbleeds Jan 12, 2024
02e97ec
use nil instead of err to rep noop
augustbleeds Jan 17, 2024
0837829
refactor tests
augustbleeds Jan 22, 2024
7c8f64f
Merge branch 'main' into augustus.gas-prices-changes
augustbleeds Jan 22, 2024
cb35a68
change noop to return 0
augustbleeds Feb 21, 2024
1e2185e
merge main
augustbleeds Feb 21, 2024
d57e9a8
try both static and noop
augustbleeds Feb 21, 2024
c9277a3
fix test
augustbleeds Feb 22, 2024
3220036
Merge branch 'main' into augustus.gas-prices-changes
augustbleeds Feb 22, 2024
9346ac9
point to new libocr
augustbleeds Feb 22, 2024
b6579c9
update libocr
augustbleeds Feb 22, 2024
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
2 changes: 1 addition & 1 deletion pkg/types/provider_median.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ type MedianProvider interface {

type PluginMedian interface {
// NewMedianFactory returns a new ReportingPluginFactory. If provider implements GRPCClientConn, it can be forwarded efficiently via proxy.
NewMedianFactory(ctx context.Context, provider MedianProvider, dataSource, juelsPerFeeCoin median.DataSource, errorLog ErrorLog) (ReportingPluginFactory, error)
NewMedianFactory(ctx context.Context, provider MedianProvider, dataSource, juelsPerFeeCoin, gasPrice median.DataSource, errorLog ErrorLog) (ReportingPluginFactory, error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is gas price used for? That seems like a chain-specific detail that shouldn't leak out here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a data source which tells us how much the gas price is on the chain during a report transmission (for chains which do not support onchain methods of retrieving gas price). It's part of the transmission structure similar to juelsPerFee so it's actually a value which is transmitted to the block-chain as a part of the median plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of it on Starknet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not all chains require it (like EVM chains), so if it is nil then ocr will not include it in a transmission

Copy link
Collaborator

Choose a reason for hiding this comment

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

But why are these low-level chain-specific details leaking out of the relayer at all? Core and chain-agnostic product code shouldn't need to concern itself with these kinds of details.

Why is a pipeline being used to retrieve this data?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do other chain families all use the word gas though? I can't actually find any other terminology, so maybe nbd 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cosmos and ethereum L2s do. I believe solana operates differently and calls them "tx fees"

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be unnecessary to include a data source that isn't used. That means we'd require node ops to include a data source that did nothing for all evm ocr2 pipelines.

The issue is precisely that whether or not to include a data source of gasPrice depends on the chain.

Can you explain how the gas price data source is going to be instantiated? We are actively working to put all chain-specific details in the relayer, and I want to make sure that we aren't taking a step back here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The data source is another pipeline so it would be instantiated here https://github.com/smartcontractkit/chainlink/pull/10519/files#diff-7358f3249bf57875121f3bcb6211722cf2bee2f9b411dfff89a1974c48c5b67eR109.

A chain either has one or it doesn't so it's either nil or it's not. There's https://github.com/smartcontractkit/chainlink-relay/pull/166/files#diff-28ea9d6d17fb7499fbbd0f83828a8335ac28eacff629256d75b261bdb2ee123aR59 also when the loopp plugin serves the underlying data source. Would this be a step backward?

This suggests that whether or not to include it is chain-specific, so any behaviour related to it should happen in the relayer.

^ If data sources were to be instantiated in the relayer, that means configuration would also occur in the relayer. The infra doesn't exist, but if it were to exist you'd pass in pipeline configs from core to the relay and relay would instantiate them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is a pipeline being used to retrieve this data?

We reimburse transmit calls on chain. In order to do that correctly a reimbursement has to be calculated. This is easy in EVM where we can access tx.gasPrice in Solidity as well as measure gasleft() at the start and end of execution. But in basically every non-EVM chain this data isn't sourceable and we need to do two things:

  • come up with reasonable constants to approximate gas use based on lots and lots of transactions
  • include a medianized gasPrice in the report (we match the juelsPerFeeCoin source usage)

We were able to avoid this second one because:

  • on Terra the gas prices were fixed and rarely changed based on some external API price list
  • on Solana the TX cost was fixed (for now, but this changes with fee markets)

This gasPrice source is needed for both Starknet and Cosmos.

}

type ReportingPluginFactory interface {
Expand Down