Make transactions compatible with Arbitrum and fix setting max gas price

[leszko]

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

Fix setting max gas price for both legacy tx and dynamic fee tx. Additionally, fix the transaction replacement for the legacy tx.

This PR uses TransactOpts to set GasFeeCap. There was already another approach merged to solve this issue by modifying TransactionManager #2111, but it caused to stop processing streams #2152, and therefore had to be reverted #2163.

Specific updates (required)

• Add TransactOpts to the Client struct
• Make all bonding contracts use TransactOpts from Client instead of the default one
• Add lock to guard any changes to TransactOpts in Client
• Update replace logic in transactionManager to cover legacy tx

How did you test each of these updates (required)

• Tested with devtool
  • Check that transactions have maxFeePerGas set to the value defined by user maxGasPrice
  • Check that streams do not stop to be processed when maxGasPrice is set
  • Checl that if maxGasPrice is reset (set to 0), then maxFeePerGas has the default value again
• Tested with Rinkeby
  • Sample transaction: https://rinkeby.etherscan.io/tx/0xe5f5e326e843e487acfb4a2ea1c6c93b9a70e08fb8c7e17104510b1588fa9356
• Tested max gas price with Arbitrum Rinkeby Testnet
  • Sample transaction: https://testnet.arbiscan.io/tx/0x5a5cbd9d807de5735ded121a6c8f92cfb403f9ff0d6729cfaf9bfac79da88858

I didn't test the transaction replacement in Arbitrum Rinkeby Testnet

Does this pull request close any open issues?

fix #2084
fix #2243

Checklist:

• Read the contribution guide
• make runs successfully
• All tests in ./test.sh pass
• README and other documentation updated
• Pending changelog updated

[yondonfu]

I think there may be a problem with this approach since the client struct currently embeds each of the binding sessions. As a result, if the caller invokes a method from a binding session that isn't explicitly defined on client, the method will be invoked on the underlying binding session that defines it [1].

For example, consider the method LivepeerTokenSession.Transfer(). At the moment, Transfer() is not defined on client, so a call to client.Transfer() (such as the one here) is equivalent to client.LivepeerTokenSession.Transfer(). With the current changes in this PR, that call should result in a nil pointer error because the TransactOpts filed in LivepeerTokenSession is not set.

I suggest thinking about a way to handle this case.

[1] https://go.dev/doc/effective_go#embedding

[leszko]

@yondonfu Good point! You're right that it can be very confusing because you'd always need to remember that you can only use functions that were overridden in the client.

I checked and you can't configure abigen to generate only get functions in the session objects. Another approach could be to not store TransactOpts in client, but to update each session with TransactOpts while calling setTransactOpts(). Something like this.

func (c *client) setTransactOpts(opts bind.TransactOpts) {
	c.transOptsMu.Lock()
	c.LivepeerTokenSession.TransactOpts = opts
	c.LivepeerTokenFaucetSession.TransactOpts = opts
	c.ServiceRegistrySession.TransactOpts = opts
	c.BondingManagerSession.TransactOpts = opts
	// ...
	c.transOptsMu.Unlock()
}

But I think it won't work because assignment operation is not thread-safe in Go and TransactOpts is used in other places in the auto-generated Go code.

Since I don't think you can embed structs partly in Go, then I think there is no better option than getting rid of embedding the sessions at all. It results in some boilerplate delegation functions, but at least it's pretty straightforward. I added this commit: 6cdba5e . Let me know if you see any better solution to it.

[yondonfu]

LGTM after resolving conflicts!

[yondonfu]

Let me know if you see any better solution to it.

Hm don't see an immediate better solution. The extra boilerplate is not great, but can live with it for now!

[leszko]

I tried to test it in Arbitrum Testnet and got the following message, which I guess is a blocker for this PR.

maxFeePerGas or maxPriorityFeePerGas specified but london is not active yet

[yondonfu]

LGTM. Let's update the changelog entry to reflect that this PR also make updates to be compatible with Arbitrum.