[action] add NewEthSigner() and pass Signer as input to rlpRawHash() and rlpSignedHash() #3967
Conversation
dustinxie
requested review from
CoderZhi,
Liuhaai,
envestcc and
millken
as code owners
Codecov Report
@@ Coverage Diff @@
## master #3967 +/- ##
==========================================
+ Coverage 75.38% 76.18% +0.80%
==========================================
Files 303 329 +26
Lines 25923 28026 +2103
==========================================
+ Hits 19541 21352 +1811
- Misses 5360 5576 +216
- Partials 1022 1098 +76
... and 3 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
api/web3server_utils.go
Outdated
| return nil, err | ||
| } | ||
| } else { | ||
| signer = types.HomesteadSigner{} |
There was a problem hiding this comment.
this is for native tx (the signature format is same as Homestead)
action/sealedenvelope.go
Outdated
| @@ -134,8 +142,8 @@ func (sealed *SealedEnvelope) loadProto(pbAct *iotextypes.Action, evmID uint32) | |||
| if err != nil { | |||
| return err | |||
| } | |||
| encoding := pbAct.GetEncoding() | |||
| switch encoding { | |||
| sealed.encoding = pbAct.GetEncoding() | |||
There was a problem hiding this comment.
even through errors are returned in line 151, 159, and 162, the encoding value of sealed has been modified. Unless we are not sure that the sealed.encoding won't be used any where else or the sealed instance won't be reused again.
Thus, we may want to introduce a local variable encoding here.
api/web3server_marshal_test.go
Outdated
| @@ -155,7 +156,8 @@ func TestBlockObjectMarshal(t *testing.T) { | |||
|
|
|||
| t.Run("BlockWithDetail", func(t *testing.T) { | |||
| raw := types.NewContractCreation(2, unit.ConvertIotxToRau(1000), 21000, unit.ConvertIotxToRau(1), []byte{}) | |||
| ethTx, err := action.RawTxToSignedTx(raw, 0, sevlp.Signature()) | |||
| signer, _ := action.NewEthSigner(iotextypes.Encoding_ETHEREUM_RLP, 0) | |||
api/web3server_utils.go
Outdated
| return nil, err | ||
| } | ||
| } else { | ||
| signer = types.HomesteadSigner{} |
There was a problem hiding this comment.
this is for native tx (the signature format is same as Homestead)
| "github.com/pkg/errors" | ||
| "golang.org/x/crypto/sha3" | ||
| ) | ||
|
|
||
| func rlpRawHash(rawTx *types.Transaction, chainID uint32) (hash.Hash256, error) { | ||
| h := types.NewEIP155Signer(big.NewInt(int64(chainID))).Hash(rawTx) | ||
| func rlpRawHash(rawTx *types.Transaction, signer types.Signer) (hash.Hash256, error) { |
There was a problem hiding this comment.
signer and hasher are of different roles
There was a problem hiding this comment.
chatted offline, it's probably not worth the effort to define a new interface Hasher for this, the code calling signer.Hash() is quite clear
api/web3server_utils.go
Outdated
| signer = types.HomesteadSigner{} | ||
| } |
There was a problem hiding this comment.
revert this change in this pr and keep only refactoring part?
There was a problem hiding this comment.
this is for native tx (the signature format is same as Homestead)
|
Kudos, SonarCloud Quality Gate passed!
|
| @@ -88,5 +88,13 @@ func DecodeRawTx(rawData string, chainID uint32) (tx *types.Transaction, sig []b | |||
| // NewEthSigner returns the proper signer for Eth-compatible tx | |||
| func NewEthSigner(txType iotextypes.Encoding, chainID uint32) (types.Signer, error) { | |||
There was a problem hiding this comment.
if the signer of iotex format could be returned, the name of this function should be updated to NewSigner
Description
Small pre-requisite code change to make (#3965) easier.
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: