Add Transaction Simulation to the TXM #12503
Conversation
amit-momin
commented
Mar 20, 2024
•
amit-momin
commented
- Add tx simulation to the TXM to allow product teams to test when a tx on ZK chains will fail due to out of counter (overflow) errors. Does not return any other types of errors. Non-zk chains should never error on this method.
- Enable tx simulation to easily adapt to chain specific methods
- Create a default case for all other chains
|
I see you updated files related to |
There was a problem hiding this comment.
Could you also update the txManager interface here if @FelixFan1992 agrees with this route?
https://github.com/smartcontractkit/chainlink/blob/develop/core/services/ocrcommon/transmitter.go#L18-L20
common/config/chaintype.go
Outdated
| @@ -19,6 +19,7 @@ const ( | |||
| ChainScroll ChainType = "scroll" | |||
| ChainWeMix ChainType = "wemix" | |||
| ChainXDai ChainType = "xdai" // Deprecated: use ChainGnosis instead | |||
| ChainZkEvm ChainType = "zkevm" | |||
There was a problem hiding this comment.
Why is this needed?
Didn't see any behavior depending on this type.
There was a problem hiding this comment.
Oh good call, missed cleaning this out after removing the zkevm specific method
| @@ -269,3 +270,12 @@ func (c *chainClient) TransactionReceipt(ctx context.Context, txHash common.Hash | |||
| func (c *chainClient) LatestFinalizedBlock(ctx context.Context) (*evmtypes.Head, error) { | |||
| return c.multiNode.LatestFinalizedBlock(ctx) | |||
| } | |||
|
|
|||
| func (c *chainClient) CheckTxValidity(ctx context.Context, from common.Address, to common.Address, data []byte) error { | |||
There was a problem hiding this comment.
How should the product understand if the received error is OOC type?
There was a problem hiding this comment.
Since we standardize the error we send back, they could use ErrOutOfCounters to determine if the error is an OOC error. I think to make it simpler for them though we'd maybe need to return a second output either another error or a flag. But I took this approach in efforts to keep the return a single field.
| @@ -269,3 +270,12 @@ func (c *chainClient) TransactionReceipt(ctx context.Context, txHash common.Hash | |||
| func (c *chainClient) LatestFinalizedBlock(ctx context.Context) (*evmtypes.Head, error) { | |||
| return c.multiNode.LatestFinalizedBlock(ctx) | |||
| } | |||
|
|
|||
| func (c *chainClient) CheckTxValidity(ctx context.Context, from common.Address, to common.Address, data []byte) *SendError { | |||
There was a problem hiding this comment.
nit: would it make sense to rename this function to something like CheckTxOverflow or something like that to highlight the Zk scope? CheckTxValidity seems a bit vague.
There was a problem hiding this comment.
I agree with this change
There was a problem hiding this comment.
Why create a separate simulatorClient instead of adding the logic directly to the existing client?
There was a problem hiding this comment.
Before when we were using the custom zkEvm method, it was so we could maintain chain specific code without crowding the client. It's possible now that it's simplified but I think there's a realistic possibility that we may need to support a custom method for a chain that's still in-progress.
There was a problem hiding this comment.
I might regret this in the future, but I still prefer to have this inside CheckTxOverflow method. Right now CheckTxOverflow is like a passthrough and has no logic. Chain client is supposed to serve exactly this purpose, which is to implement chain-specific logic that can't go inside multinode. As a reader, it would be easier for me to just read the entire thing under one file instead of having to understand why tx_simulator was created. I don't have a hard stance on this, so if you guys think it's cleaner as it is, then I'm ok keeping it this way.
There was a problem hiding this comment.
I would maybe argue that chain client is more so EVM specific code. So if something requires chain specific code where different EVM chains have different implementation, my opinion is we'd want to separate that from the chain client. That being said, we skipped on using the zkevm chain specific code here so I'm not against moving this to the chain client. We could always separate this out again later when there's a need. But before I do, let me get agreement from others.
There was a problem hiding this comment.
I would be ok with anything here.
Either make some changes here, or wait for the final solution to be clearer wrt other ZK chains, and then make the appropriate code changes.
There was a problem hiding this comment.
I'd prefer to wait to avoid another back and forth while product teams are waiting. But I agree we'll have more changes come to this code in the near future when we can make this decision.
| @@ -269,3 +270,12 @@ func (c *chainClient) TransactionReceipt(ctx context.Context, txHash common.Hash | |||
| func (c *chainClient) LatestFinalizedBlock(ctx context.Context) (*evmtypes.Head, error) { | |||
| return c.multiNode.LatestFinalizedBlock(ctx) | |||
| } | |||
|
|
|||
| func (c *chainClient) CheckTxValidity(ctx context.Context, from common.Address, to common.Address, data []byte) *SendError { | |||
There was a problem hiding this comment.
How is CheckTxValidity going to be used by products? Do they need a SendError code or a bool response would be sufficient enough. The reason I'm asking is because with this implementation the caller is forced to know about the SendError type, whereas it might be better if it gets abstracted completely.
There was a problem hiding this comment.
Prashant and I talked this over a bit in the thread. I don't think this could be handled with just a bool or a basic error type. There might be some additional errors that the product teams could check for when simulating like service unavailable or timeout. If it was a simple bool or error type, they wouldn't be able to differentiate between these which is why we opted to return SendError. That way they could check the specific types of errors with the built in methods like IsOutOfCounters()
There was a problem hiding this comment.
There's an opportunity to clean up our error responses handling across ALL of chain client interface.
Can't be tackled in this PR, but likely a bigger effort into how to handle errors in standard way, for all of the Chain Client interfaces. Maybe a new ticket.
Created one here: https://smartcontract-it.atlassian.net/browse/BCI-2863
|
