-
Notifications
You must be signed in to change notification settings - Fork 838
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
refactor NewTx
and NewContract
arguments
#1402
Conversation
NewTx
and NewContract
argument listNewTx
and NewContract
arguments
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1402 +/- ##
==========================================
- Coverage 72.40% 72.32% -0.08%
==========================================
Files 260 260
Lines 17725 17723 -2
==========================================
- Hits 12833 12818 -15
- Misses 4322 4336 +14
+ Partials 570 569 -1
|
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.
Nice PR @facs95. One suggestion: Can we define a few test EvmTxArgs
types in a testutils
directory, then use the same objects everywhere?
There's a lot of duplicate code in the current state, and doing this would simplify the implementation a lot.
tx.From = addr.Hex() | ||
err = tx.Sign(suite.ethSigner, testutil.NewSigner(privKey)) | ||
suite.Require().NoError(err) | ||
|
||
tx2 := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 1, &to, big.NewInt(10), 1000, big.NewInt(1), nil, nil, nil, nil) | ||
ethTxParamsNonce1 := &evmtypes.EvmTxArgs{ |
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.
Maybe we can wrap these with a helper function?
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.
Requested changes to prevent additional breaking changes. Don't forget adding the changelog entry
@austinchandra Yes that's exactly the idea! Will be done on a follow up PR for ease of reviewing |
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.
ACK, pending changelog entry and removing NewTxContract
function
Description
This PR refactors
NewTx
andNewContract
function atx/evm/types/msg.go
. Instead of passing an enourmous amount of arguments, we accomplish the same goal through a new structEvmTxArgs
.This will make the test Txs helper function migration into a global common util package easier and cleaner.
The PR aims to make things simpler for the testing suite refactor, specifically the global tx util functions that are so repetitive throughout the codebase.
This PR does NOT change any tests functionality nor tries to do a refractor on this tests. Just changes tests to pass the arguments through the struct.
Test refactor and migration into a common package will come on a following PR.
Closes #XXX