Add reward caller (using transcoder-to-reward caller mapping direction)

[SidestreamSweatyPumpkin]

What does this pull request do? Explain your changes. (required)

This PR adds a new feature to the livepeer protocol which will allow transcoders to set a single address they trust to claim the rewards every on round without requiring the main wallet to stay unlocked.

Specific updates (required)

• contracts/bonding/BondingManager.sol contract is modified to include
  • New transcoderToRewardCaller public mapping
  • New setRewardCaller function to set or unset a rewardCaller
  • New rewardForTranscoder, rewardForTranscoderWithHint functions to claim rewards using rewardCaller account
• test/unit/BondingManager.js test is modified to include new "reward delegation" section
• yarn test:coverage:check command is added to the CI to insure 100% test coverage

How did you test each of these updates (required)

A new "reward delegation" section contains tests that ensure:

• A transcoder is able to to call the reward even if RewardCaller is set for them
• If a RewardCaller is set, the account is able to call rewardForTranscoder
• If the RewardCaller is unset, it no longer able to call rewardForTranscoder
• Checkpoint logs reward recipient, not the RewardCaller

Does this pull request close any open issues?

No

Checklist:

• README and other documentation updated
  • No relevant documentation found, the changes are backwards compatible
• All tests using yarn test pass

[SidestreamSweatyPumpkin]

Additional notes on the implementation

Note-01:

It is possible for a transcoder to set the RewardCaller as themself and call functions designated for RewardCallers. By doing so, the transcoder itself will be able to use the rewardForTranscoder and rewardForTranscoderWithHint. But other than being able to call both functions, there is no security risk identified. Note: we didn't include this edge case in the tests as this is not intended use-case.

Note-02:

It is possible for a TranscoderA to set the TranscoderB as RewardCaller. In this case, TranscoderB will be able to claim reward for themself by calling reward() and can claim reward for TranscoderA by calling rewardForTranscoder(TranscoderA). We didn't identify any security risk associated with this edge case.

Note-03:

In this implementation it is possible that multiple different transcoders set the same address as their RewardCaller. It will allow the same RewardCaller to call rewardForTranscoder(TranscoderA), rewardForTranscoder(TranscoderB) or even use multi-call functionality to save on gas.

Note-04:

It is not part of the current implementation, but it's possible to add a functionality to self-revoke RewardCaller. Currently, only the transcoder which set the RewardCaller can unset it by calling setRewardCaller with a different (or a zero) address.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00000%. Comparing base (6e6b452) to head (99c0008).
⚠️ Report is 1 commits behind head on delta.

Additional details and impacted files

@@               Coverage Diff               @@
##                delta         #648   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files              29           29           
  Lines            1338         1349   +11     
  Branches          225          226    +1     
===============================================
+ Hits             1338         1349   +11     

Files with missing lines	Coverage Δ
contracts/bonding/BondingManager.sol	100.00000% <100.00000%> (ø)

---

Continue to review full report in Codecov by Harness.

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

Files with missing lines	Coverage Δ
contracts/bonding/BondingManager.sol	100.00000% <100.00000%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rickstaa]

Non-blocking test suggestion: the auth require is never tested with a wrong caller while one is set (mapping = A, sender = B). It's somewhat implicit in the existing unset-caller test given the current implementation, but a specific test would guard future changes — it pins the check to "only the authorized address" vs "anyone once set":

it("should fail if caller is not the reward caller set by the transcoder", async () => {
    await bondingManager.connect(transcoder).setRewardCaller(nonTranscoder.address)
    await expect(
        bondingManager.connect(signers[2]).rewardForTranscoder(transcoder.address)
    ).to.be.revertedWith("caller must be a reward caller set by the transcoder")
})

[rickstaa]

@SidestreamSweatyPumpkin Implementation is correct, my personal security and code review found no exploits and no griefing angles (full notes in Notion). Summary: setter is self-scoped and revocable; delegated reward is economically identical to a self-call; no new external call sites; storage layout append-safe; all BondingManager unit tests pass.

Biggest finding was that we need to patch the explorer HistoryView in the explorer with the upgrade to ensure reward events still show up correctly for people using this new function. The change is not breaking the subgraph or go-livepeer. For go-livepeer as discussed before the changes are only addative.

Left some minor comments regarding wording and tests.

[SidestreamCrunchyCarrot]

Sanity checks:

• ✅ No changes unrelated to the PR description
• ✅ CI pass at the latest commit 99c0008
• ✅ Hardhat-based tests run locally on the latest block (939 passing (6m))
• ✅ Test coverage is at 100%

[dob]

I'm comfortable with the logic introduced in this change. Though I did leave one open question about the modifiers on the rewardWithHint function to address.

[dob]

Why did we do away with the whenSystemNotPaused, currentRoundInitialized, autoCheckpoint modifiers on this function?

[SidestreamCrunchyCarrot]

They didn't go away. All of those modifiers are being called in the underlying private function _rewardWithHint on line 926:

protocol/contracts/bonding/BondingManager.sol

Lines 922 to 926 in 99c0008

	function _rewardWithHint(
	address _transcoder,
	address _newPosPrev,
	address _newPosNext
	) private whenSystemNotPaused currentRoundInitialized autoCheckpoint(_transcoder) {

This way, both rewardWithHint and rewardForTranscoderWithHint has the same modifiers.

[rickstaa]

To add to this, keeping it in the parent function may increase the risk of introducing side effects if rewardWithHint or any of the other external functions change.