-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add Transaction Simulation to the TXM #12503
Merged
Merged
Changes from 25 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
5a87d87
Added tx simulator to maintain chain specific simulation methods
amit-momin bdb9fef
Fixed linting
amit-momin 46b13be
Fixed linting and regenerated config docs
amit-momin 141dfea
Generated mock
amit-momin 4835554
Fixed config tests
amit-momin 27922a3
Merge branch 'develop' into zk-tx-simulation
amit-momin befeaf0
Moved the tx simulator to the chain client
amit-momin fbdd110
Removed client from Txm struct
amit-momin 2f7078f
Removed config from test helper
amit-momin fc70e7f
Added tests and logging
amit-momin 389e18f
Added changeset
amit-momin 37b783e
Fixed multinode test
amit-momin cdc58b2
Merge branch 'develop' into zk-tx-simulation
amit-momin e126805
Fixed linting
amit-momin 0c22a6f
Merge branch 'develop' into zk-tx-simulation
amit-momin c7eae50
Fixed comment
amit-momin 53efe22
Added test for non-OOC error
amit-momin 8ad2510
Reduced context initializations in tests
amit-momin 3f53867
Updated to account for all types of OOC errors
amit-momin ce8a3c7
Removed custom zk counter method and simplified error handling
amit-momin d7605cc
Removed zkevm chain type
amit-momin f301d43
Changed simulate tx method return object
amit-momin eb34bc7
Cleaned up stale comments
amit-momin 68de594
Removed unused error message
amit-momin 1a25270
Changed zk overflow validation method name
amit-momin 6798e4d
Reverted method name change
amit-momin 87c8eac
Merge branch 'develop' into zk-tx-simulation
amit-momin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"chainlink": minor | ||
--- | ||
|
||
Added a tx simulation feature to the chain client to enable testing for zk out-of-counter (OOC) errors |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
package client | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/ethereum/go-ethereum" | ||
"github.com/ethereum/go-ethereum/common/hexutil" | ||
|
||
"github.com/smartcontractkit/chainlink-common/pkg/logger" | ||
"github.com/smartcontractkit/chainlink/v2/common/config" | ||
) | ||
|
||
type simulatorClient interface { | ||
CallContext(ctx context.Context, result interface{}, method string, args ...interface{}) error | ||
} | ||
|
||
// ZK chains can return an out-of-counters error | ||
// This method allows a caller to determine if a tx would fail due to OOC error by simulating the transaction | ||
// Used as an entry point in case custom simulation is required across different chains | ||
func SimulateTransaction(ctx context.Context, client simulatorClient, lggr logger.SugaredLogger, chainType config.ChainType, msg ethereum.CallMsg) *SendError { | ||
err := simulateTransactionDefault(ctx, client, msg) | ||
return NewSendError(err) | ||
} | ||
|
||
// eth_estimateGas returns out-of-counters (OOC) error if the transaction would result in an overflow | ||
func simulateTransactionDefault(ctx context.Context, client simulatorClient, msg ethereum.CallMsg) error { | ||
var result hexutil.Big | ||
return client.CallContext(ctx, &result, "eth_estimateGas", toCallArg(msg), "pending") | ||
} | ||
|
||
func toCallArg(msg ethereum.CallMsg) interface{} { | ||
arg := map[string]interface{}{ | ||
"from": msg.From, | ||
"to": msg.To, | ||
} | ||
if len(msg.Data) > 0 { | ||
arg["input"] = hexutil.Bytes(msg.Data) | ||
} | ||
if msg.Value != nil { | ||
arg["value"] = (*hexutil.Big)(msg.Value) | ||
} | ||
if msg.Gas != 0 { | ||
arg["gas"] = hexutil.Uint64(msg.Gas) | ||
} | ||
if msg.GasPrice != nil { | ||
arg["gasPrice"] = (*hexutil.Big)(msg.GasPrice) | ||
} | ||
if msg.GasFeeCap != nil { | ||
arg["maxFeePerGas"] = (*hexutil.Big)(msg.GasFeeCap) | ||
} | ||
if msg.GasTipCap != nil { | ||
arg["maxPriorityFeePerGas"] = (*hexutil.Big)(msg.GasTipCap) | ||
} | ||
return arg | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
package client_test | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/ethereum/go-ethereum" | ||
"github.com/stretchr/testify/require" | ||
"github.com/tidwall/gjson" | ||
|
||
"github.com/smartcontractkit/chainlink-common/pkg/logger" | ||
|
||
"github.com/smartcontractkit/chainlink/v2/core/chains/evm/client" | ||
"github.com/smartcontractkit/chainlink/v2/core/internal/cltest" | ||
"github.com/smartcontractkit/chainlink/v2/core/internal/testutils" | ||
) | ||
|
||
func TestSimulateTx_Default(t *testing.T) { | ||
t.Parallel() | ||
|
||
fromAddress := testutils.NewAddress() | ||
toAddress := testutils.NewAddress() | ||
ctx := testutils.Context(t) | ||
|
||
amit-momin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
t.Run("returns without error if simulation passes", func(t *testing.T) { | ||
wsURL := testutils.NewWSServer(t, &cltest.FixtureChainID, func(method string, params gjson.Result) (resp testutils.JSONRPCResponse) { | ||
switch method { | ||
case "eth_subscribe": | ||
resp.Result = `"0x00"` | ||
resp.Notify = headResult | ||
return | ||
case "eth_unsubscribe": | ||
resp.Result = "true" | ||
return | ||
case "eth_estimateGas": | ||
resp.Result = `"0x100"` | ||
} | ||
return | ||
}).WSURL().String() | ||
|
||
ethClient := mustNewChainClient(t, wsURL) | ||
err := ethClient.Dial(ctx) | ||
require.NoError(t, err) | ||
|
||
msg := ethereum.CallMsg{ | ||
From: fromAddress, | ||
To: &toAddress, | ||
Data: []byte("0x00"), | ||
} | ||
sendErr := client.SimulateTransaction(ctx, ethClient, logger.TestSugared(t), "", msg) | ||
require.Empty(t, sendErr) | ||
}) | ||
|
||
t.Run("returns error if simulation returns zk out-of-counters error", func(t *testing.T) { | ||
wsURL := testutils.NewWSServer(t, &cltest.FixtureChainID, func(method string, params gjson.Result) (resp testutils.JSONRPCResponse) { | ||
switch method { | ||
case "eth_subscribe": | ||
resp.Result = `"0x00"` | ||
resp.Notify = headResult | ||
return | ||
case "eth_unsubscribe": | ||
resp.Result = "true" | ||
return | ||
case "eth_estimateGas": | ||
resp.Error.Code = -32000 | ||
resp.Result = `"0x100"` | ||
resp.Error.Message = "not enough keccak counters to continue the execution" | ||
} | ||
return | ||
}).WSURL().String() | ||
|
||
ethClient := mustNewChainClient(t, wsURL) | ||
err := ethClient.Dial(ctx) | ||
require.NoError(t, err) | ||
|
||
msg := ethereum.CallMsg{ | ||
From: fromAddress, | ||
To: &toAddress, | ||
Data: []byte("0x00"), | ||
} | ||
sendErr := client.SimulateTransaction(ctx, ethClient, logger.TestSugared(t), "", msg) | ||
require.Equal(t, true, sendErr.IsOutOfCounters()) | ||
}) | ||
|
||
t.Run("returns without error if simulation returns non-OOC error", func(t *testing.T) { | ||
wsURL := testutils.NewWSServer(t, &cltest.FixtureChainID, func(method string, params gjson.Result) (resp testutils.JSONRPCResponse) { | ||
switch method { | ||
case "eth_subscribe": | ||
resp.Result = `"0x00"` | ||
resp.Notify = headResult | ||
return | ||
case "eth_unsubscribe": | ||
resp.Result = "true" | ||
return | ||
case "eth_estimateGas": | ||
resp.Error.Code = -32000 | ||
resp.Error.Message = "something went wrong" | ||
} | ||
return | ||
}).WSURL().String() | ||
|
||
ethClient := mustNewChainClient(t, wsURL) | ||
err := ethClient.Dial(ctx) | ||
require.NoError(t, err) | ||
|
||
msg := ethereum.CallMsg{ | ||
From: fromAddress, | ||
To: &toAddress, | ||
Data: []byte("0x00"), | ||
} | ||
sendErr := client.SimulateTransaction(ctx, ethClient, logger.TestSugared(t), "", msg) | ||
require.Equal(t, false, sendErr.IsOutOfCounters()) | ||
}) | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why create a separate simulatorClient instead of adding the logic directly to the existing client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might regret this in the future, but I still prefer to have this inside
CheckTxOverflow
method. Right nowCheckTxOverflow
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 whytx_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.