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

Enable Orchestrator to set pricing by broadcaster ETH address #2592

Merged
merged 34 commits into from
Oct 4, 2022
Merged

Enable Orchestrator to set pricing by broadcaster ETH address #2592

merged 34 commits into from
Oct 4, 2022

Conversation

eliteprox
Copy link
Contributor

@eliteprox eliteprox commented Sep 18, 2022

What does this pull request do? Explain your changes. (required)
Enables orchestrator to set specific pricePerPixel and pricePerUnit by the broadcaster's ETH address.

Specific updates (required)

  • Adds new CLI option -pricePerBroadcaster to livepeer.go
  • Adds new HTTP handler /setPriceForBroadcaster to handlers.go and webserver.go
  • Adds new livepeer_cli menu option to set broadcaster prices.
  • Updates tests for pricing

How did you test each of these updates (required)

All tests were ran on testnet with linux using multiple broadcasters and an orchestrator node compiled with the changes.

  • Ran livepeer as orchestrator with -pricePerUnit 5, -pixelsPerUnit 1 and -pricePerBroadcaster set to {"broadcasters":[{"ethaddress":"0x6E98a769926f55A65e34066E67198f6B68e4B166","priceperunit":0,"pixelsperunit":1},{"ethaddress":"0xF115A3dfd39778A6719Ace6AA77A2aD228e90329","priceperunit":1200,"pixelsperunit":1}]}

  • Ran livepeer as orchestrator with -pricePerUnit 0, -pixelsPerUnit 1 and -pricePerBroadcaster {"broadcasters":[{"ethaddress":"0x6E98a769926f55A65e34066E67198f6B68e4B166","priceperunit":1200,"pixelsperunit":1}]}

  • Tested the same using a json file -pricePerBroadcaster broadcasterprices.json.

  • Set prices using curl -X POST -d "pixelsPerUnit=1&pricePerUnit=9&broadcasterEthAddr=0xEB62188121725A605f76a97791a1C69B4BdB435D" http://127.0.0.1:7920/setPriceForBroadcaster

  • Confirmed correct price sent to broadcaster by verifying payment ticket logs.

  • Confirmed no deposit required for broadcaster when broadcaster uses -maxPricePerUnit 0 and orchestrator sets broadcaster's ETH address to a pricePerUnit of 0.

  • Confirmed orchestrator's -pricePerUnit and -pricePerPixel continue to function as the default price when a broadcaster is not assigned a specific price via -pricePerBroadcaster, -freeStream or /setPricePerBroadcaster.

Does this pull request close any open issues?

Checklist:

@eliteprox eliteprox changed the title Enable Orchestrator to defines pricing by broadcaster ETH address Enable Orchestrator to define pricing by broadcaster ETH address Sep 18, 2022
@eliteprox eliteprox changed the title Enable Orchestrator to define pricing by broadcaster ETH address Enable Orchestrator to set pricing by broadcaster ETH address Sep 18, 2022
@eliteprox eliteprox marked this pull request as ready for review September 20, 2022 11:37
@leszko leszko self-requested a review September 20, 2022 14:10
@leszko
Copy link
Contributor

leszko commented Sep 20, 2022

Thank you for the PR! We'll review it in the next 1-2 weeks.

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.

Thank you for the PR. Added a few comments.

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
core/orchestrator.go Outdated Show resolved Hide resolved
cmd/livepeer_cli/wizard_transcoder.go Outdated Show resolved Hide resolved
server/mediaserver.go Outdated Show resolved Hide resolved
common/types.go Outdated Show resolved Hide resolved
server/mediaserver.go Outdated Show resolved Hide resolved
server/handlers.go 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.

Added some super minor comments. Other than that, could you address the following 2 points?

  1. The following GH Action is failing, could you check?

  2. I've played a little bit with your change and set the price per broadcaster, but then I don't see it in the status when checked from livepeer_cli. Also when I do the following curl call, I don't see the broadcaster prices. Could you check?

$ curl localhost:1936/status
{"Manifests":{},"InternalManifests":{},"StreamInfo":{},"OrchestratorPool":[],"OrchestratorPoolInfos":null,"Version":"0.5.34-129325e0","GolangRuntimeVersion":"go1.18.2","GOArch":"arm64","GOOS":"darwin","RegisteredTranscodersNumber":0,"RegisteredTranscoders":[],"LocalTranscoding":true,"BroadcasterPrices":{}}

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 2, 2022

Codecov Report

Merging #2592 (6cb541c) into master (dc6e79d) will decrease coverage by 0.12246%.
The diff coverage is 41.17647%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2592         +/-   ##
===================================================
- Coverage   56.44931%   56.32685%   -0.12246%     
===================================================
  Files             88          88                 
  Lines          18909       19030        +121     
===================================================
+ Hits           10674       10719         +45     
- Misses          7656        7728         +72     
- Partials         579         583          +4     
Impacted Files Coverage Δ
cmd/livepeer_cli/livepeer_cli.go 0.00000% <ø> (ø)
cmd/livepeer_cli/wizard_stats.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_transcoder.go 0.00000% <0.00000%> (ø)
common/types.go 0.00000% <ø> (ø)
cmd/livepeer/starter/starter.go 2.90262% <31.81818%> (+0.61254%) ⬆️
server/handlers.go 53.83244% <66.66667%> (+0.31645%) ⬆️
cmd/livepeer/livepeer.go 51.46199% <100.00000%> (+0.28551%) ⬆️
common/readfromfile.go 89.28571% <100.00000%> (ø)
core/livepeernode.go 78.00000% <100.00000%> (+5.50000%) ⬆️
core/orchestrator.go 77.93103% <100.00000%> (+0.15324%) ⬆️
... and 3 more

Continue to review full report at Codecov.

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

Impacted Files Coverage Δ
cmd/livepeer_cli/livepeer_cli.go 0.00000% <ø> (ø)
cmd/livepeer_cli/wizard_stats.go 0.00000% <0.00000%> (ø)
cmd/livepeer_cli/wizard_transcoder.go 0.00000% <0.00000%> (ø)
common/types.go 0.00000% <ø> (ø)
cmd/livepeer/starter/starter.go 2.90262% <31.81818%> (+0.61254%) ⬆️
server/handlers.go 53.83244% <66.66667%> (+0.31645%) ⬆️
cmd/livepeer/livepeer.go 51.46199% <100.00000%> (+0.28551%) ⬆️
common/readfromfile.go 89.28571% <100.00000%> (ø)
core/livepeernode.go 78.00000% <100.00000%> (+5.50000%) ⬆️
core/orchestrator.go 77.93103% <100.00000%> (+0.15324%) ⬆️
... and 3 more

@leszko
Copy link
Contributor

leszko commented Oct 3, 2022

@eliteprox Please re-request review or just comment here when you think that your PR is ready for review.

@leszko
Copy link
Contributor

leszko commented Oct 3, 2022

btw. I've seen the GH Action started to pass 🎉

@ad-astra-video
Copy link
Contributor

@leszko We are adding some tests so the CodeCov reports pass. We had some tests when submitting but it wants a few more.

@eliteprox eliteprox requested a review from leszko October 3, 2022 13:57
@eliteprox
Copy link
Contributor Author

eliteprox commented Oct 3, 2022

Added some super minor comments. Other than that, could you address the following 2 points?

1. The [following GH Action](https://github.com/livepeer/go-livepeer/actions/runs/3155409577/jobs/5141365984) is failing, could you check?

2. I've played a little bit with your change and set the price per broadcaster, but then I don't see it in the status when checked from `livepeer_cli`. Also when I do the following curl call, I don't see the broadcaster prices. Could you check?
$ curl localhost:1936/status
{"Manifests":{},"InternalManifests":{},"StreamInfo":{},"OrchestratorPool":[],"OrchestratorPoolInfos":null,"Version":"0.5.34-129325e0","GolangRuntimeVersion":"go1.18.2","GOArch":"arm64","GOOS":"darwin","RegisteredTranscodersNumber":0,"RegisteredTranscoders":[],"LocalTranscoding":true,"BroadcasterPrices":{}}

Tests are passing and this now works:
curl localhost:7920/status

{"Manifests":{},"InternalManifests":{},"StreamInfo":{},"OrchestratorPool":[],"OrchestratorPoolInfos":null,"Version":"undefined","GolangRuntimeVersion":"go1.18.1","GOArch":"amd64","GOOS":"linux","RegisteredTranscodersNumber":0,"RegisteredTranscoders":[],"LocalTranscoding":false,"BroadcasterPrices":{"0xeb62188121725a605f76a97791a1c69b4bdb435d":"9","default":"2"}

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.

Revert unrelated change

server/mediaserver_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.

Add new line at the end of starter_test.go

@leszko leszko self-requested a review October 4, 2022 08:04
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 👍

@leszko
Copy link
Contributor

leszko commented Oct 4, 2022

Thanks a lot for this PR @eliteprox

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