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.

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

cmd/livepeer: Use price feed watcher for dynamic pricePerPixel #2981

Merged
merged 34 commits into from
Mar 27, 2024

Conversation

victorges
Copy link
Member

@victorges victorges commented Mar 21, 2024

What does this pull request do? Explain your changes. (required)
This uses the Chainlink price feed watcher component created on #2972 to
support dynamic transcode pricing for Broadcasters and Orchestrators. It works
by simply updating the LivepeerNode.SetBasePrice or server.BroadcastCfg.SetMaxPrice
when there are price updates on the price feed.

Specific updates (required)

  • Make the price flags strings to support currency specification
  • Create new flag for the price feed address
  • Start price update loop for Bs and Os

How did you test each of these updates (required)

  • go test
  • Ran B/O and checked price update happens as expected

Does this pull request close any open issues?
Implements ENG-1726

Checklist:

Makefile: Use mockgen binary from tool dependencies

eth/contracts: Add chainlink interfaces source

Makefile: Generate Chainlink contracts ABI

tools: Add abigen tool to repo

eth/contracts: Generate chainlink bindings

Makefile: Fix abigen bindings generation

Revert everything abigen

Turns out there's already bindings exported from the Chainlink lib.

go.mod: Add chainlink library

eth/watchers: Add pricefeed watcher

eth/watchers: Clean-up event watching code

eth/watchers: Improve price tracking

Revert "go.mod: Add chainlink library"

This reverts commit ac415bd.

Revert "Revert everything abigen"

This reverts commit b7c40b1.

eth/contracts: Gen bindings for proxy iface

eth/watchers: Use local bindings for contracts

eth/watchers: Simplify event subs logic

eth/watchers: Simplify&optimize truncated ticker

eth/watchers: Update decimals on fetch

eth/watchers: Improve handling of decimals

eth/watchers: Fix price rat creation

eth/watchers: Make sure we use UTC on truncated timer

eth/contracts/chainlink: Generate only V3 contract bindings

eth/watchers: Watch PriceFeed only with polling

eth/watchers: Add a retry logic on price update

eth/watchers: Use clog instead of fmt.Printf
This will make the code more testable.
Gosh that was much harder than I thought
Currently ignoring the currency value.
@victorges victorges changed the base branch from master to vg/feat/usd-price-per-unit March 21, 2024 15:59
@victorges victorges changed the title Vg/feat/use usd price per unit cmd/livepeer: Use price feed watcher for dynamic pricePerPixel Mar 21, 2024
@victorges victorges marked this pull request as ready for review March 21, 2024 22:01
@victorges victorges marked this pull request as draft March 22, 2024 00:09
@victorges victorges force-pushed the vg/feat/use-usd-price-per-unit branch 3 times, most recently from 12060d7 to 644e680 Compare March 23, 2024 23:04
@victorges victorges force-pushed the vg/feat/use-usd-price-per-unit branch from 644e680 to 87907f2 Compare March 23, 2024 23:16
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Show resolved Hide resolved
cmd/livepeer/starter/starter.go Show resolved Hide resolved
@victorges victorges marked this pull request as ready for review March 25, 2024 17:59
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added some comments. It looks good, but I think it requires some more manual testing. Unfortunately go-livepeer is a beast and we don't have too many automated tests. Therefore, we need to check manually at least the following scenarios:

  1. Set up local dev (or ask some public O to test it on Mainnet):
  • O: Set different prices per Broadcasters read from a file
  • O: Set dynamically prices per Broadcasters
  • O: Set dynamically default price from wei to USD and back to wei and back to USD
  • B: Set max price from commandline
  • B: Set max price dynamically
  1. Check if automated tests work in Catalyst (enough to send a draft PR with the update go-livepeer version to your commit hash)

core/livepeernode.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Show resolved Hide resolved
cmd/livepeer/starter/starter.go Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/livepeer.go Show resolved Hide resolved
cmd/livepeer/starter/starter.go Show resolved Hide resolved
core/autoconvertedprice.go Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
eth/watchers/pricefeedwatcher_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Parse it directlty as a big.Rat from a raw string, like I was
doing for pricePerUnit in some places.
Turns out tests were not running on my branch due to base branch
Base automatically changed from vg/feat/usd-price-per-unit to master March 27, 2024 18:57
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 55.41796% with 144 lines in your changes are missing coverage. Please review.

Project coverage is 57.47690%. Comparing base (ef5d789) to head (32c6f14).

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2981         +/-   ##
===================================================
+ Coverage   57.24231%   57.47690%   +0.23459%     
===================================================
  Files             91          92          +1     
  Lines          15513       15695        +182     
===================================================
+ Hits            8880        9021        +141     
- Misses          6040        6078         +38     
- Partials         593         596          +3     
Files Coverage Δ
cmd/livepeer/livepeer.go 49.66443% <100.00000%> (+0.34011%) ⬆️
core/livepeernode.go 68.18182% <100.00000%> (+5.02393%) ⬆️
server/broadcast.go 80.38793% <100.00000%> (+0.27982%) ⬆️
core/autoconvertedprice.go 93.44262% <93.44262%> (ø)
server/handlers.go 55.74289% <82.97872%> (+0.76408%) ⬆️
cmd/livepeer_cli/wizard_stats.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_broadcast.go 0.00000% <0.00000%> (ø)
eth/watchers/pricefeedwatcher.go 67.30769% <63.26531%> (+26.34383%) ⬆️
cmd/livepeer_cli/wizard_transcoder.go 0.00000% <0.00000%> (ø)
cmd/livepeer/starter/starter.go 7.19352% <30.33708%> (+1.97613%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef5d789...32c6f14. Read the comment docs.

Files Coverage Δ
cmd/livepeer/livepeer.go 49.66443% <100.00000%> (+0.34011%) ⬆️
core/livepeernode.go 68.18182% <100.00000%> (+5.02393%) ⬆️
server/broadcast.go 80.38793% <100.00000%> (+0.27982%) ⬆️
core/autoconvertedprice.go 93.44262% <93.44262%> (ø)
server/handlers.go 55.74289% <82.97872%> (+0.76408%) ⬆️
cmd/livepeer_cli/wizard_stats.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_broadcast.go 0.00000% <0.00000%> (ø)
eth/watchers/pricefeedwatcher.go 67.30769% <63.26531%> (+26.34383%) ⬆️
cmd/livepeer_cli/wizard_transcoder.go 0.00000% <0.00000%> (ø)
cmd/livepeer/starter/starter.go 7.19352% <30.33708%> (+1.97613%) ⬆️

Found out that's officially supported precision on the
discovery logic, so let's reflect that here.
@victorges victorges merged commit 706ec33 into master Mar 27, 2024
18 checks passed
@victorges victorges deleted the vg/feat/use-usd-price-per-unit branch March 27, 2024 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants