feat(eth-rpc): Conversion types and functions between Ethereum txs and blocks and Tendermint ones. #1856
Conversation
…nagement and event distribution
…d blocks and Tendermint ones.
WalkthroughThis update enhances Ethereum integration with features like in-memory EventBus for real-time event management, and new Ethereum RPC functionalities. It also includes significant refactoring, renaming the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
eth/rpc/types/query_client.go
Outdated
| // Base app does not support queries for height less than or equal to 1, and | ||
| // the base app uses 0 indexing. | ||
| // | ||
| // Ethereum uses 1 indexing for the intial block height, therefore <= 2 means |
There was a problem hiding this comment.
Correct the typographical error in the comment.
- // Ethereum uses 1 indexing for the intial block height, therefore <= 2 means
+ // Ethereum uses 1 indexing for the initial block height, therefore <= 2 meansCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // Ethereum uses 1 indexing for the intial block height, therefore <= 2 means | |
| // Ethereum uses 1 indexing for the initial block height, therefore <= 2 means |
eth/rpc/pubsub/pubsub.go
Outdated
| // AddTopic adds a new topic with the specified name and message source | ||
| func (m *memEventBus) AddTopic(name string, src <-chan coretypes.ResultEvent) error { | ||
| m.topicsMux.RLock() | ||
| _, ok := m.topics[name] | ||
| m.topicsMux.RUnlock() | ||
|
|
||
| if ok { | ||
| return errors.New("topic already registered") | ||
| } | ||
|
|
||
| m.topicsMux.Lock() | ||
| m.topics[name] = src | ||
| m.topicsMux.Unlock() | ||
|
|
||
| go m.publishTopic(name, src) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The AddTopic method correctly handles the addition of new topics, including thread-safe checks and the initialization of the publishing goroutine. However, consider handling the potential race condition between checking if a topic exists and adding it. This could be mitigated by using a single write lock for the entire operation.
- m.topicsMux.RLock()
- _, ok := m.topics[name]
- m.topicsMux.RUnlock()
- if ok {
+ m.topicsMux.Lock()
+ if _, ok := m.topics[name]; ok {
+ m.topicsMux.Unlock()
return errors.New("topic already registered")
}
- m.topicsMux.Lock()
m.topics[name] = src
m.topicsMux.Unlock()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // AddTopic adds a new topic with the specified name and message source | |
| func (m *memEventBus) AddTopic(name string, src <-chan coretypes.ResultEvent) error { | |
| m.topicsMux.RLock() | |
| _, ok := m.topics[name] | |
| m.topicsMux.RUnlock() | |
| if ok { | |
| return errors.New("topic already registered") | |
| } | |
| m.topicsMux.Lock() | |
| m.topics[name] = src | |
| m.topicsMux.Unlock() | |
| go m.publishTopic(name, src) | |
| return nil | |
| } | |
| // AddTopic adds a new topic with the specified name and message source | |
| func (m *memEventBus) AddTopic(name string, src <-chan coretypes.ResultEvent) error { | |
| m.topicsMux.Lock() | |
| if _, ok := m.topics[name]; ok { | |
| m.topicsMux.Unlock() | |
| return errors.New("topic already registered") | |
| } | |
| m.topics[name] = src | |
| m.topicsMux.Unlock() | |
| go m.publishTopic(name, src) | |
| return nil | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1856 +/- ##
==========================================
+ Coverage 68.59% 71.97% +3.37%
==========================================
Files 176 183 +7
Lines 10091 10514 +423
==========================================
+ Hits 6922 7567 +645
+ Misses 2747 2475 -272
- Partials 422 472 +50
|
Unique-Divine
force-pushed
the
ud/evm-rpc
branch
from
f2621ba to
372df12
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
eth/eip712/preprocess_test.go (1)
Line range hint
19-20: Handling ofnilinput should be checked before other conditions to avoid potential null pointer dereferences.- } else if i == nil { + if i == nil {eth/types/chain_id_test.go (1)
54-56: Ensure that the test case description accurately reflects the test scenario. The description "cannot invalid base" seems grammatically incorrect and unclear.
| } else if i == nil { | ||
| return sdkmath.Int{}, fmt.Errorf("received nil pointer for *big.Int") |
There was a problem hiding this comment.
Properly handle nil input before other conditions to prevent null pointer dereferences.
- } else if i == nil {
+ if i == nil {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| } else if i == nil { | |
| return sdkmath.Int{}, fmt.Errorf("received nil pointer for *big.Int") | |
| if i == nil { | |
| return sdkmath.Int{}, fmt.Errorf("received nil pointer for *big.Int") |
There was a problem hiding this comment.
Actionable comments posted: 13
Out of diff range and nitpick comments (11)
eth/assert.go (1)
Line range hint
18-18: The functionValidateAddressusescommon.IsHexAddressfor validation, which is appropriate. However, consider adding a comment to clarify what constitutes a valid Ethereum address for readers unfamiliar with Ethereum's address format.x/evm/codec.go (2)
Line range hint
14-14: Consider adding unit tests for theinitfunction to ensure that the legacy Amino codec is registered correctly.Would you like me to help by creating a test case for this function?
Line range hint
49-49: The registration of transaction types should be accompanied by documentation explaining each type's use case and structure.Consider adding comments detailing the purpose and structure of
DynamicFeeTx,AccessListTx, andLegacyTx.eth/state_encoder_test.go (1)
87-88: The panic handling inTestEncoderEthAddris appropriate, but consider adding a specific test case to document what conditions lead to a panic.It would be beneficial to add a comment or a test case that explicitly states under what conditions a panic is expected in
TestEncoderEthAddr.eth/assert_test.go (1)
36-57: The functionIsZeroAddressis tested well. Consider adding a performance benchmark to assess the function's efficiency, especially if used frequently in the codebase.Consider adding a performance benchmark for
IsZeroAddressto evaluate its efficiency under load.eth/crypto/hd/algorithm_test.go (1)
Line range hint
90-108: Refactor to use table-driven tests for better maintainability.+ tests := []struct { + name string + path string + expectError bool + }{ + {"valid path", eth.BIP44HDPath, false}, + {"invalid path", "44'/60'/0'/0/0", true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + bz, err := EthSecp256k1.Derive()(mnemonic, keyring.DefaultBIP39Passphrase, tc.path) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + require.NotEmpty(t, bz) + } + }) + }This test case can be refactored into a table-driven format to enhance readability and maintainability. This approach allows easy addition of new test scenarios and makes the test cases more structured.
x/evm/errors.go (1)
2-2: Ensure the package comment is present for better code documentation.Consider adding a package comment at the beginning of the file to explain the purpose and scope of the
evmpackage. This enhances code readability and maintainability, especially for new contributors.x/evm/legacy_tx.go (1)
Line range hint
129-196: RefactorAsEthereumDatato improve error handling.+ if err := tx.Validate(); err != nil { + return nil, err + } return &gethcore.LegacyTx{ Nonce: tx.GetNonce(), GasPrice: tx.GetGasPrice(), Gas: tx.GetGas(), To: tx.GetTo(), Value: tx.GetValue(), Data: tx.GetData(), V: v, R: r, S: s, }, nilConsider adding a validation step in the
AsEthereumDatamethod to ensure that the transaction data is valid before constructing thegethcore.LegacyTxobject. This can prevent issues downstream where invalid data might be processed.eth/rpc/block.go (1)
1-1: Ensure the file has a proper header comment.Add a header comment at the beginning of the file to explain its purpose and the types of functions it contains. This helps in maintaining a good documentation standard and aids new developers in understanding the file's role in the project.
x/evm/chain_config.go (1)
2-2: Consider adding more detailed comments for each configuration parameter.Enhance the documentation within the
ChainConfigstructure by adding detailed comments for each parameter, explaining its purpose and impact. This improves code readability and helps maintainers and developers understand the configuration options better.proto/eth/evm/v1/tx.proto (1)
12-12: Enhance documentation for each protobuf message.Add detailed comments for each protobuf message and field to explain their purpose and usage. This is particularly important for complex fields or those with non-obvious implications, as it aids in understanding and maintaining the code.
| @@ -363,7 +363,7 @@ func (s *StateDB) Suicide(addr common.Address) bool { | |||
| // - Add the contents of the optional tx access list (2930) | |||
| // | |||
| // This method should only be called if Yolov3/Berlin/2929+2930 is applicable at the current number. | |||
| func (s *StateDB) PrepareAccessList(sender common.Address, dst *common.Address, precompiles []common.Address, list ethtypes.AccessList) { | |||
| func (s *StateDB) PrepareAccessList(sender common.Address, dst *common.Address, precompiles []common.Address, list gethcore.AccessList) { | |||
There was a problem hiding this comment.
Review the implementation of PrepareAccessList for potential optimization.
Consider refactoring the method to reduce redundancy and improve performance, especially in scenarios with large access lists.
| evm.QueryClient | ||
| } | ||
|
|
||
| // NewQueryClient creates a new gRPC query client |
There was a problem hiding this comment.
The NewQueryClient function is implemented correctly to instantiate a QueryClient. However, ensure that the TODO comment is addressed promptly as it relates to critical testing.
Would you like me to help with implementing the test for GetProof as mentioned in the TODO comment?
| @@ -61,7 +61,7 @@ func TestKeyring(t *testing.T) { | |||
| require.NoError(t, err) | |||
| require.Equal(t, string(EthSecp256k1Type), pubKey.Type()) | |||
|
|
|||
| hdPath := ethtypes.BIP44HDPath | |||
| hdPath := eth.BIP44HDPath | |||
There was a problem hiding this comment.
Consider handling potential data races when setting environment variables.
+ sync.Mutex.Lock()
os.Setenv(hdWalletFixEnv, "true")
wallet, err := NewFromMnemonic(mnemonic)
os.Setenv(hdWalletFixEnv, "")
+ sync.Mutex.Unlock()Using environment variables in tests can lead to data races when tests are run in parallel. Consider using a mutex to synchronize access to environment variables or refactor to use instance-specific configuration.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| hdPath := eth.BIP44HDPath | |
| hdPath := eth.BIP44HDPath | |
| sync.Mutex.Lock() | |
| os.Setenv(hdWalletFixEnv, "true") | |
| wallet, err := NewFromMnemonic(mnemonic) | |
| os.Setenv(hdWalletFixEnv, "") | |
| sync.Mutex.Unlock() |
| @@ -1,5 +1,5 @@ | |||
| // Copyright (c) 2023-2024 Nibi, Inc. | |||
| package types | |||
| package evm | |||
There was a problem hiding this comment.
Consider adding unit tests for NewExecErrorWithReason.
It appears that there are no unit tests for the NewExecErrorWithReason function. Would you like me to help by writing some test cases or should I open a GitHub issue to track this task?
| @@ -72,7 +72,7 @@ func (tx *LegacyTx) GetChainID() *big.Int { | |||
| } | |||
|
|
|||
| // GetAccessList returns nil | |||
| func (tx *LegacyTx) GetAccessList() geth.AccessList { | |||
| func (tx *LegacyTx) GetAccessList() gethcore.AccessList { | |||
There was a problem hiding this comment.
Add support for access lists if applicable.
Legacy transactions do not support access lists, but if there is a requirement to simulate or convert to a type that supports them, consider implementing this functionality. Would you like assistance with this?
| @@ -1,5 +1,5 @@ | |||
| // Copyright (c) 2023-2024 Nibi, Inc. | |||
| package types | |||
| package evm | |||
There was a problem hiding this comment.
Consider adding more comprehensive tests for access list transactions.
Given the complexity of access list transactions, consider adding more comprehensive tests to cover various edge cases and error conditions. Would you like help in writing these tests or should I open a GitHub issue to track this task?
| func (s *SuiteSafeMath) TestSafeInt64() { | ||
| tests := []struct { | ||
| name string | ||
| input uint64 | ||
| expectErr bool | ||
| }{ | ||
| { | ||
| name: "Valid conversion", | ||
| input: maxInt64, // Maximum value for int64 | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "Invalid conversion causes overflow", | ||
| input: uint64(maxInt64) + 1, // Exceeds maximum int64 value | ||
| expectErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| s.Run(tc.name, func() { | ||
| result, err := eth.SafeInt64(tc.input) | ||
| if tc.expectErr { | ||
| s.Error(err, "Expected an error due to overflow but did not get one") | ||
| } else { | ||
| s.NoError(err, "Did not expect an error but got one") | ||
| s.Equal(int64(tc.input), result, "The results should match") | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests for SafeInt64 handle edge cases well. Consider adding a test for the boundary condition where the input is just below the overflow threshold.
Would you like me to add a test case for the boundary condition in SafeInt64?
| func filterImplsForEth(implTypeUrls []string) []string { | ||
| typeUrls := []string{} | ||
| for _, typeUrl := range implTypeUrls { | ||
| if strings.Contains(typeUrl, "eth") { | ||
| typeUrls = append(typeUrls, typeUrl) | ||
| } | ||
| } | ||
| return typeUrls | ||
| } |
There was a problem hiding this comment.
The function filterImplsForEth is used to filter implementations. Consider adding a test specifically for this function to ensure its correctness across a range of inputs.
Would you like me to help by creating a test case for the filterImplsForEth function?
| // TestConcurrentAccess: Tests the system's behavior under high concurrency, | ||
| // specifically ensuring that the lock can handle multiple locking and unlocking | ||
| // operations on the same address without leading to race conditions or | ||
| // deadlocks. | ||
| func (s *SuiteAddrLocker) TestConcurrentAccess() { | ||
| locker := &rpc.AddrLocker{} | ||
| addr := common.HexToAddress("0x789") | ||
| var wg sync.WaitGroup | ||
|
|
||
| // Spawn 100 goroutines, each locking and unlocking the same address. | ||
| // Each routine will hod the lock briefly to simulate work done during the | ||
| // lock (like an Ethereum query). | ||
| for i := 0; i < 100; i++ { | ||
| wg.Add(1) | ||
| go func() { | ||
| locker.LockAddr(addr) | ||
| time.Sleep(time.Millisecond * 5) // Simulate work | ||
| locker.UnlockAddr(addr) | ||
| wg.Done() | ||
| }() | ||
| } | ||
|
|
||
| // Cleanup: Wait for all goroutines to complete | ||
| wg.Wait() | ||
| } |
There was a problem hiding this comment.
The TestConcurrentAccess test is essential for ensuring that the locking mechanism can handle high levels of concurrency without issues. Consider adding more varied scenarios to this test to cover more complex interactions.
Would you like me to add more scenarios to the TestConcurrentAccess test to cover more complex interactions?
| // OverrideAccount indicates the overriding fields of account during the execution of | ||
| // a message call. | ||
| // Note, state and stateDiff can't be specified at the same time. If state is | ||
| // set, message execution will only use the data in the given state. Otherwise | ||
| // if statDiff is set, all diff will be applied first and then execute the call | ||
| // message. | ||
| type OverrideAccount struct { | ||
| Nonce *hexutil.Uint64 `json:"nonce"` | ||
| Code *hexutil.Bytes `json:"code"` | ||
| Balance **hexutil.Big `json:"balance"` | ||
| State *map[common.Hash]common.Hash `json:"state"` | ||
| StateDiff *map[common.Hash]common.Hash `json:"stateDiff"` | ||
| } |
There was a problem hiding this comment.
The OverrideAccount type allows for state overrides during message calls. It's crucial to ensure that this functionality is secure and does not introduce potential vulnerabilities.
Consider adding security audits or reviews specifically for the functionality provided by OverrideAccount to ensure it does not introduce security vulnerabilities.
| func (s *SuiteStorage) TestStorageValidate() { | ||
| testCases := []struct { | ||
| name string | ||
| storage Storage | ||
| wantPass bool | ||
| }{ | ||
| { | ||
| name: "valid storage", | ||
| storage: Storage{ | ||
| NewStateFromEthHashes(common.BytesToHash([]byte{1, 2, 3}), common.BytesToHash([]byte{1, 2, 3})), | ||
| }, | ||
| wantPass: true, | ||
| }, | ||
| { | ||
| name: "empty storage key bytes", | ||
| storage: Storage{ | ||
| {Key: ""}, | ||
| }, | ||
| wantPass: false, | ||
| }, | ||
| { | ||
| name: "duplicated storage key", | ||
| storage: Storage{ | ||
| {Key: common.BytesToHash([]byte{1, 2, 3}).String()}, | ||
| {Key: common.BytesToHash([]byte{1, 2, 3}).String()}, | ||
| }, | ||
| wantPass: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| tc := tc | ||
| err := tc.storage.Validate() | ||
| if tc.wantPass { | ||
| s.NoError(err, tc.name) | ||
| } else { | ||
| s.Error(err, tc.name) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider using descriptive variable names or adding comments for byte arrays used as keys to enhance readability.
- NewStateFromEthHashes(common.BytesToHash([]byte{1, 2, 3}), common.BytesToHash([]byte{1, 2, 3})),
+ keyBytes := []byte{1, 2, 3}
+ NewStateFromEthHashes(common.BytesToHash(keyBytes), common.BytesToHash(keyBytes)),Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (s *SuiteStorage) TestStorageValidate() { | |
| testCases := []struct { | |
| name string | |
| storage Storage | |
| wantPass bool | |
| }{ | |
| { | |
| name: "valid storage", | |
| storage: Storage{ | |
| NewStateFromEthHashes(common.BytesToHash([]byte{1, 2, 3}), common.BytesToHash([]byte{1, 2, 3})), | |
| }, | |
| wantPass: true, | |
| }, | |
| { | |
| name: "empty storage key bytes", | |
| storage: Storage{ | |
| {Key: ""}, | |
| }, | |
| wantPass: false, | |
| }, | |
| { | |
| name: "duplicated storage key", | |
| storage: Storage{ | |
| {Key: common.BytesToHash([]byte{1, 2, 3}).String()}, | |
| {Key: common.BytesToHash([]byte{1, 2, 3}).String()}, | |
| }, | |
| wantPass: false, | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| tc := tc | |
| err := tc.storage.Validate() | |
| if tc.wantPass { | |
| s.NoError(err, tc.name) | |
| } else { | |
| s.Error(err, tc.name) | |
| } | |
| } | |
| } | |
| func (s *SuiteStorage) TestStorageValidate() { | |
| testCases := []struct { | |
| name string | |
| storage Storage | |
| wantPass bool | |
| }{ | |
| { | |
| name: "valid storage", | |
| storage: Storage{ | |
| keyBytes := []byte{1, 2, 3} | |
| NewStateFromEthHashes(common.BytesToHash(keyBytes), common.BytesToHash(keyBytes)), | |
| }, | |
| wantPass: true, | |
| }, | |
| { | |
| name: "empty storage key bytes", | |
| storage: Storage{ | |
| {Key: ""}, | |
| }, | |
| wantPass: false, | |
| }, | |
| { | |
| name: "duplicated storage key", | |
| storage: Storage{ | |
| {Key: common.BytesToHash([]byte{1, 2, 3}).String()}, | |
| {Key: common.BytesToHash([]byte{1, 2, 3}).String()}, | |
| }, | |
| wantPass: false, | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| tc := tc | |
| err := tc.storage.Validate() | |
| if tc.wantPass { | |
| s.NoError(err, tc.name) | |
| } else { | |
| s.Error(err, tc.name) | |
| } | |
| } | |
| } |
| func (s *MsgsSuite) TestMsgEthereumTx_Constructor() { | ||
| evmTx := &evm.EvmTxArgs{ | ||
| Nonce: 0, | ||
| To: &s.to, | ||
| GasLimit: 100000, | ||
| Input: []byte("test"), | ||
| } | ||
| msg := evm.NewTx(evmTx) | ||
|
|
||
| // suite.Require().Equal(msg.Data.To, suite.to.Hex()) | ||
| s.Require().Equal(msg.Route(), evm.RouterKey) | ||
| s.Require().Equal(msg.Type(), evm.TypeMsgEthereumTx) | ||
| // suite.Require().NotNil(msg.To()) | ||
| s.Require().Equal(msg.GetMsgs(), []sdk.Msg{msg}) | ||
| s.Require().Panics(func() { msg.GetSigners() }) | ||
| s.Require().Panics(func() { msg.GetSignBytes() }) | ||
|
|
||
| evmTx2 := &evm.EvmTxArgs{ | ||
| Nonce: 0, | ||
| GasLimit: 100000, | ||
| Input: []byte("test"), | ||
| } | ||
| msg = evm.NewTx(evmTx2) | ||
| s.Require().NotNil(msg) |
There was a problem hiding this comment.
Consider removing or uncommenting the commented-out assertions to clarify their necessity for the test.
- // suite.Require().Equal(msg.Data.To, suite.to.Hex())
- // suite.Require().NotNil(msg.To())Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (s *MsgsSuite) TestMsgEthereumTx_Constructor() { | |
| evmTx := &evm.EvmTxArgs{ | |
| Nonce: 0, | |
| To: &s.to, | |
| GasLimit: 100000, | |
| Input: []byte("test"), | |
| } | |
| msg := evm.NewTx(evmTx) | |
| // suite.Require().Equal(msg.Data.To, suite.to.Hex()) | |
| s.Require().Equal(msg.Route(), evm.RouterKey) | |
| s.Require().Equal(msg.Type(), evm.TypeMsgEthereumTx) | |
| // suite.Require().NotNil(msg.To()) | |
| s.Require().Equal(msg.GetMsgs(), []sdk.Msg{msg}) | |
| s.Require().Panics(func() { msg.GetSigners() }) | |
| s.Require().Panics(func() { msg.GetSignBytes() }) | |
| evmTx2 := &evm.EvmTxArgs{ | |
| Nonce: 0, | |
| GasLimit: 100000, | |
| Input: []byte("test"), | |
| } | |
| msg = evm.NewTx(evmTx2) | |
| s.Require().NotNil(msg) | |
| func (s *MsgsSuite) TestMsgEthereumTx_Constructor() { | |
| evmTx := &evm.EvmTxArgs{ | |
| Nonce: 0, | |
| To: &s.to, | |
| GasLimit: 100000, | |
| Input: []byte("test"), | |
| } | |
| msg := evm.NewTx(evmTx) | |
| s.Require().Equal(msg.Route(), evm.RouterKey) | |
| s.Require().Equal(msg.Type(), evm.TypeMsgEthereumTx) | |
| s.Require().Equal(msg.GetMsgs(), []sdk.Msg{msg}) | |
| s.Require().Panics(func() { msg.GetSigners() }) | |
| s.Require().Panics(func() { msg.GetSignBytes() }) | |
| evmTx2 := &evm.EvmTxArgs{ | |
| Nonce: 0, | |
| GasLimit: 100000, | |
| Input: []byte("test"), | |
| } | |
| msg = evm.NewTx(evmTx2) | |
| s.Require().NotNil(msg) | |
| } |
Unique-Divine
force-pushed
the
ud/evm-rpc
branch
from
d995990 to
14f8a2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
cmd/nibid/cmd/root.go (1)
Line range hint
51-78: Ensure consistency in command descriptions and error handling inNewRootCmd.- cmd.SetOut(cmd.OutOrStdout()) - cmd.SetErr(cmd.ErrOrStderr()) + // Ensure standard output and error streams are set for the command + cmd.SetOut(cmd.OutOrStdout()) + cmd.SetErr(cmd.ErrOrStderr())
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.modis excluded by!**/*.mod,!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (10)
- cmd/nibid/cmd/root.go (5 hunks)
- cmd/nibid/cmd/testnet.go (1 hunks)
- eth/rpc/rpc.go (1 hunks)
- eth/rpc/rpc_test.go (1 hunks)
- eth/state_encoder.go (2 hunks)
- x/evm/README.md (1 hunks)
- x/evm/evmtest/eth.go (1 hunks)
- x/evm/evmtest/eth_test.go (1 hunks)
- x/evm/params.go (3 hunks)
- x/evm/tx.go (10 hunks)
Files skipped from review due to trivial changes (2)
- cmd/nibid/cmd/testnet.go
- x/evm/README.md
Files skipped from review as they are similar to previous changes (4)
- eth/rpc/rpc.go
- eth/state_encoder.go
- x/evm/params.go
- x/evm/tx.go
Additional comments not posted (1)
x/evm/evmtest/eth_test.go (1)
20-33: Ensure proper error handling inTestSampleFns.Verification successful
The error handling in the function
TestSampleFnswithineth_test.gohas been verified. The use ofs.NoErroris appropriately implemented to check for errors after operations that could potentially fail. This confirms that the error handling is as expected based on the review comment.
s.NoError(err)is used after creating a new Ethereum transaction message to ensure no errors occurred.s.NoError(ethTxMsg.ValidateBasic())is used to check each Ethereum transaction message in a loop, ensuring each message is valid.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper error handling in TestSampleFns. # Test: Search for error handling patterns in TestSampleFns. Expect: Proper error handling. rg --type go 's.NoError' 'x/evm/evmtest/eth_test.go'Length of output: 104
| func (s *SuiteRPC) TestRawTxToEthTx() { | ||
| type TestCase struct { | ||
| tx cmt.Tx | ||
| clientCtx client.Context | ||
| wantErr string | ||
| } | ||
| type TestCaseSetupFn = func() TestCase | ||
|
|
||
| for _, tcSetup := range []TestCaseSetupFn{ | ||
| func() TestCase { | ||
| _, _, clientCtx := evmtest.NewEthTxMsgAsCmt(s.T()) | ||
| txBz := []byte("tx") | ||
| return TestCase{ | ||
| tx: txBz, // invalid bytes | ||
| clientCtx: clientCtx, // valid clientCtx | ||
| wantErr: "failed to unmarshal JSON", | ||
| } | ||
| }, | ||
| func() TestCase { | ||
| txBz, _, clientCtx := evmtest.NewEthTxMsgAsCmt(s.T()) | ||
| return TestCase{ | ||
| tx: txBz, // valid bytes | ||
| clientCtx: clientCtx, // valid clientCtx | ||
| wantErr: "", // happy | ||
| } | ||
| }, | ||
| } { | ||
| tc := tcSetup() | ||
| ethTxs, err := rpc.RawTxToEthTx(tc.clientCtx, tc.tx) | ||
| if tc.wantErr != "" { | ||
| s.Require().ErrorContains(err, tc.wantErr, "ethTxs: %s", ethTxs) | ||
| continue | ||
| } | ||
| s.Require().NoError(err, "ethTxs: %s", ethTxs) | ||
| } |
There was a problem hiding this comment.
Refactor test cases in TestRawTxToEthTx for clarity and maintainability.
- for _, tcSetup := range []TestCaseSetupFn{
- func() TestCase {
- _, _, clientCtx := evmtest.NewEthTxMsgAsCmt(s.T())
- txBz := []byte("tx")
- return TestCase{
- tx: txBz, // invalid bytes
- clientCtx: clientCtx, // valid clientCtx
- wantErr: "failed to unmarshal JSON",
- }
- },
- func() TestCase {
- txBz, _, clientCtx := evmtest.NewEthTxMsgAsCmt(s.T())
- return TestCase{
- tx: txBz, // valid bytes
- clientCtx: clientCtx, // valid clientCtx
- wantErr: "", // happy
- }
- },
- } {
+ testCases := []TestCase{
+ {
+ tx: []byte("tx"), // invalid bytes
+ clientCtx: evmtest.NewEthTxMsgAsCmt(s.T()).clientCtx, // valid clientCtx
+ wantErr: "failed to unmarshal JSON",
+ },
+ {
+ tx: evmtest.NewEthTxMsgAsCmt(s.T()).txBz, // valid bytes
+ clientCtx: evmtest.NewEthTxMsgAsCmt(s.T()).clientCtx, // valid clientCtx
+ wantErr: "", // happy
+ },
+ }
+ for _, tc := range testCases {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (s *SuiteRPC) TestRawTxToEthTx() { | |
| type TestCase struct { | |
| tx cmt.Tx | |
| clientCtx client.Context | |
| wantErr string | |
| } | |
| type TestCaseSetupFn = func() TestCase | |
| for _, tcSetup := range []TestCaseSetupFn{ | |
| func() TestCase { | |
| _, _, clientCtx := evmtest.NewEthTxMsgAsCmt(s.T()) | |
| txBz := []byte("tx") | |
| return TestCase{ | |
| tx: txBz, // invalid bytes | |
| clientCtx: clientCtx, // valid clientCtx | |
| wantErr: "failed to unmarshal JSON", | |
| } | |
| }, | |
| func() TestCase { | |
| txBz, _, clientCtx := evmtest.NewEthTxMsgAsCmt(s.T()) | |
| return TestCase{ | |
| tx: txBz, // valid bytes | |
| clientCtx: clientCtx, // valid clientCtx | |
| wantErr: "", // happy | |
| } | |
| }, | |
| } { | |
| tc := tcSetup() | |
| ethTxs, err := rpc.RawTxToEthTx(tc.clientCtx, tc.tx) | |
| if tc.wantErr != "" { | |
| s.Require().ErrorContains(err, tc.wantErr, "ethTxs: %s", ethTxs) | |
| continue | |
| } | |
| s.Require().NoError(err, "ethTxs: %s", ethTxs) | |
| } | |
| func (s *SuiteRPC) TestRawTxToEthTx() { | |
| type TestCase struct { | |
| tx cmt.Tx | |
| clientCtx client.Context | |
| wantErr string | |
| } | |
| type TestCaseSetupFn = func() TestCase | |
| testCases := []TestCase{ | |
| { | |
| tx: []byte("tx"), // invalid bytes | |
| clientCtx: evmtest.NewEthTxMsgAsCmt(s.T()).clientCtx, // valid clientCtx | |
| wantErr: "failed to unmarshal JSON", | |
| }, | |
| { | |
| tx: evmtest.NewEthTxMsgAsCmt(s.T()).txBz, // valid bytes | |
| clientCtx: evmtest.NewEthTxMsgAsCmt(s.T()).clientCtx, // valid clientCtx | |
| wantErr: "", // happy | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| ethTxs, err := rpc.RawTxToEthTx(tc.clientCtx, tc.tx) | |
| if tc.wantErr != "" { | |
| s.Require().ErrorContains(err, tc.wantErr, "ethTxs: %s", ethTxs) | |
| continue | |
| } | |
| s.Require().NoError(err, "ethTxs: %s", ethTxs) | |
| } |
x/evm/evmtest/eth.go
Outdated
| // NewEthTxMsgAsCmt: Helper that returns an Ethereum tx msg converted into | ||
| // tx bytes used in the Consensus Engine. | ||
| func NewEthTxMsgAsCmt(t *testing.T) ( | ||
| txBz cmt.Tx, | ||
| ethTxMsgs []*evm.MsgEthereumTx, | ||
| clientCtx client.Context, | ||
| ) { | ||
| // Build a TxBuilder that can understand Ethereum types | ||
| encCfg := encoding.MakeConfig(app.ModuleBasics) | ||
| evm.RegisterInterfaces(encCfg.InterfaceRegistry) | ||
| eth.RegisterInterfaces(encCfg.InterfaceRegistry) | ||
| txConfig := encCfg.TxConfig | ||
| clientCtx = client.Context{ | ||
| TxConfig: txConfig, | ||
| InterfaceRegistry: encCfg.InterfaceRegistry, | ||
| } | ||
| txBuilder := clientCtx.TxConfig.NewTxBuilder() | ||
|
|
||
| // Build a consensus tx with a few Eth tx msgs | ||
| ethTxMsgs = NewEthTxMsgs(3) | ||
| assert.NoError(t, | ||
| txBuilder.SetMsgs(ethTxMsgs[0], ethTxMsgs[1], ethTxMsgs[2]), | ||
| ) | ||
| tx := txBuilder.GetTx() | ||
| txBz, err := clientCtx.TxConfig.TxEncoder()(tx) | ||
| assert.NoError(t, err) | ||
| return txBz, ethTxMsgs, clientCtx | ||
| } |
There was a problem hiding this comment.
Optimize NewEthTxMsgAsCmt by reducing redundancy in transaction message creation.
- ethTxMsgs = NewEthTxMsgs(3)
- assert.NoError(t,
- txBuilder.SetMsgs(ethTxMsgs[0], ethTxMsgs[1], ethTxMsgs[2]),
- )
+ ethTxMsgs = NewEthTxMsgs(3)
+ assert.NoError(t, txBuilder.SetMsgs(ethTxMsgs...))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // NewEthTxMsgAsCmt: Helper that returns an Ethereum tx msg converted into | |
| // tx bytes used in the Consensus Engine. | |
| func NewEthTxMsgAsCmt(t *testing.T) ( | |
| txBz cmt.Tx, | |
| ethTxMsgs []*evm.MsgEthereumTx, | |
| clientCtx client.Context, | |
| ) { | |
| // Build a TxBuilder that can understand Ethereum types | |
| encCfg := encoding.MakeConfig(app.ModuleBasics) | |
| evm.RegisterInterfaces(encCfg.InterfaceRegistry) | |
| eth.RegisterInterfaces(encCfg.InterfaceRegistry) | |
| txConfig := encCfg.TxConfig | |
| clientCtx = client.Context{ | |
| TxConfig: txConfig, | |
| InterfaceRegistry: encCfg.InterfaceRegistry, | |
| } | |
| txBuilder := clientCtx.TxConfig.NewTxBuilder() | |
| // Build a consensus tx with a few Eth tx msgs | |
| ethTxMsgs = NewEthTxMsgs(3) | |
| assert.NoError(t, | |
| txBuilder.SetMsgs(ethTxMsgs[0], ethTxMsgs[1], ethTxMsgs[2]), | |
| ) | |
| tx := txBuilder.GetTx() | |
| txBz, err := clientCtx.TxConfig.TxEncoder()(tx) | |
| assert.NoError(t, err) | |
| return txBz, ethTxMsgs, clientCtx | |
| } | |
| // NewEthTxMsgAsCmt: Helper that returns an Ethereum tx msg converted into | |
| // tx bytes used in the Consensus Engine. | |
| func NewEthTxMsgAsCmt(t *testing.T) ( | |
| txBz cmt.Tx, | |
| ethTxMsgs []*evm.MsgEthereumTx, | |
| clientCtx client.Context, | |
| ) { | |
| // Build a TxBuilder that can understand Ethereum types | |
| encCfg := encoding.MakeConfig(app.ModuleBasics) | |
| evm.RegisterInterfaces(encCfg.InterfaceRegistry) | |
| eth.RegisterInterfaces(encCfg.InterfaceRegistry) | |
| txConfig := encCfg.TxConfig | |
| clientCtx = client.Context{ | |
| TxConfig: txConfig, | |
| InterfaceRegistry: encCfg.InterfaceRegistry, | |
| } | |
| txBuilder := clientCtx.TxConfig.NewTxBuilder() | |
| // Build a consensus tx with a few Eth tx msgs | |
| ethTxMsgs = NewEthTxMsgs(3) | |
| assert.NoError(t, txBuilder.SetMsgs(ethTxMsgs...)) | |
| tx := txBuilder.GetTx() | |
| txBz, err := clientCtx.TxConfig.TxEncoder()(tx) | |
| assert.NoError(t, err) | |
| return txBz, ethTxMsgs, clientCtx | |
| } |
Purpose / Abstract
CODECOV_TOKENthat needs to be used for uploads now.Tip
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores