colonyNetwork icon indicating copy to clipboard operation
colonyNetwork copied to clipboard

Make the metatransaction broadcaster more robust

Open area opened this issue 2 years ago • 0 comments

Based on #1070 (as this branch is being used by the frontend team), so no merging for now.

The main issue with the metatransaction broadcaster in the real world is to do with nonces for the account doing the broadcasting. With the current (committed, though not deployed) implementation, two requests for a metatransaction at the same time would end up with the same nonce, and one wouldn't be accepted. This is obviously bad, so I started using the experimental NonceManager from Ethers, which gives us finer control over nonces. Basically, it signs transactions with sequential nonces as requests come in, regardless of what the RPC node it is connected to tells it.

There are issues with this - if the nonce gets out-of-sync, for whatever reason, things start going wrong. So I've added logic to the NonceManager around the nonce getting out of sync to try and allow it to recover. It also keeps track of transactions its signed, and if it seems like the node it's connected to has forgotten about them, will re-send them (with the same nonce). If it somehow gets ahead of itself, it should also go back and 'fill in' missing nonces.

There is also some generic logic around resetting the transaction count if an error message is seen with the word 'nonce' in it. We also apply some belt-and-braces logic by adding a queue, so only one broadcast is serviced at a time. It's possible that this will slow down sending metatransactions, but until we get complaints about it, it's something else that should help nonces going wrong.

This PR also adds support for the EIP2612 transactions we use when interacting with (bridged) tokens.

We also add some logic around allowing finalizeMotion where we can - basically, if the motion is for something that we'd allow, we will pay for it.

setAuthority on a token was a tricky one. As it takes an address, I didn't want to allow all setAuthority calls, as in principle that argument could be used to pass anything, and we don't want to pay for everything that a user could do on GnosisChain. I ended up assuming that the user will always be going through flows in the frontend, and so we should be able to check the passed address is an authority that was just deployed by the network.

Finally, setOwner has similar logic - when going through flows in the frontend, we only ever call setOwner with the parameter of a colony, so that can be easily allowed (or not).

area avatar Jul 22 '22 16:07 area