[action] add `address` and `hash` field in SealedEnvelope #3420

saitofun · 2022-05-30T04:59:21Z

Description

- perf: [action] improve SealedEnvelope accessing `hash`, `hashErr` and `srcAddress`
- test: [action] improve TestSealedEnvelope_Basic TestSealedEnvelope_Proto and TestActionDeserializer
- feat: [action] add `ErrInvalidSigLen` and `ErrUnknownEncType` error func for maintain test and error better

Fixes #2989

Type of change

Please delete options that are not relevant.

Code refactor or improvement
New feature (non-breaking change which adds functionality)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

[] My code follows the style guidelines of this project
[] I have performed a self-review of my code
[] I have commented my code, particularly in hard-to-understand areas
[] I have made corresponding changes to the documentation
[] My changes generate no new warnings
[] I have added tests that prove my fix is effective or that my feature works
[] New and existing unit tests pass locally with my changes
[] Any dependent changes have been merged and published in downstream modules

CLAassistant · 2022-05-30T04:59:25Z

All committers have signed the CLA.

dustinxie · 2022-05-30T18:31:17Z

action/sealedenvelope.go

+ hv, err := sealed.calcHash()
+ sealed.hash = &Hash256Result{hv, err}
+ }
+ return sealed.hash.Value()


return sealed.hash.Hash256, sealed.hash.error would suffice

dustinxie · 2022-05-30T18:32:31Z

action/sealedenvelope.go

+ error
+}
+
+func (h *Hash256Result) Value() (hash.Hash256, error) {


no need for this func, see comments below

dustinxie · 2022-05-30T18:33:21Z

action/sealedenvelope.go

+ hash *Hash256Result
+}
+
+type Hash256Result struct {


hash256Result, this is internal data struct

Remove this struct

dustinxie · 2022-05-30T18:41:22Z

action/sealedenvelope_test.go

@@ -165,6 +165,7 @@ func TestSealedEnvelope_Proto(t *testing.T) {
 {1, "d5dc789026c12cc69f1ea7997fbe0aa1bcc02e85176848c7b2ecf4da6b4560d0"},
 } {
 se.encoding = v.enc
+ req.NoError(se.LoadProto(se.Proto()))


this should not be needed?
se.Hash() below will trigger the if hash == nil code path and test its correctness

dustinxie · 2022-05-30T18:43:47Z

action/sealedenvelope_test.go

@@ -35,7 +35,7 @@ func TestSealedEnvelope_Basic(t *testing.T) {
 {0, "322884fb04663019be6fb461d9453827487eafdd57b4de3bd89a7d77c9bf8395"},
 {1, "80af7840d73772d3022d8bdc46278fb755352e5e9d5f2a1f12ee7ec4f1ea98e9"},
 } {
- se, err := createSealedEnvelope(v.id)
+ se, err := createSealedEnvelopeWithSignature(v.id, _signByte)


should not need this? see comments below

dustinxie · 2022-05-30T18:44:18Z

action/sealedenvelope_test.go

+ se.signature = _validSig
+ if err = se.LoadProto(se.Proto()); err != nil {
+ return se, err
+ }


we don't need this? see comments below

dustinxie · 2022-05-30T18:45:09Z

action/sealedenvelope_test.go

 return se, err
 }
+
+func createSealedEnvelopeWithSignature(chainID uint32, sig []byte) (SealedEnvelope, error) {


we don't need to add this func? if there's a need (for test) about signature, just directly set the signature (as done in tests now)

dustinxie · 2022-05-30T18:48:27Z

action/sealedenvelope.go

@@ -69,6 +93,13 @@ func (sealed *SealedEnvelope) Hash() (hash.Hash256, error) {
 // SrcPubkey returns the source public key
 func (sealed *SealedEnvelope) SrcPubkey() crypto.PublicKey { return sealed.srcPubkey }

+func (sealed *SealedEnvelope) Address() address.Address {


need a comment like // Address() xxx to satisfy go lint
you run make lint local to make sure no lint error

SenderAddress

change to SenderAddress, it's a better/clear name

CoderZhi · 2022-05-31T18:33:25Z

action/sealedenvelope.go

@@ -69,6 +93,13 @@ func (sealed *SealedEnvelope) Hash() (hash.Hash256, error) {
 // SrcPubkey returns the source public key
 func (sealed *SealedEnvelope) SrcPubkey() crypto.PublicKey { return sealed.srcPubkey }

+func (sealed *SealedEnvelope) Address() address.Address {


SenderAddress

CoderZhi · 2022-05-31T18:35:45Z

action/sealedenvelope.go

+ hashValue, err := sealed.calcHash()
+ sealed.hash = &Hash256Result{hashValue, err}
+ sealed.address = srcPub.Address()


dustinxie · 2022-06-02T17:08:21Z

action/action_deserializer_test.go

+ _ = se.Address()
+ // after Hash() and Address() was invoked, the two value are equal
+ NewWithT(t).Expect(validated).To(Equal(se))
+ })


revert these, we don't want to introduce a new testing style, pls only do minimum necessary changes to make existing test pass

as we discussed, please revert unnecessary changes, only need these 3 lines to make the test pass

_, err = se.Hash() r.NoError(err) se.SenderAddress() // after Hash() and Address() was invoked, the two value are equal

dustinxie · 2022-06-02T17:09:15Z

action/const.go


 // vars
 var (
- ErrAddress = errors.New("invalid address")
+ ErrAddress = errors.New("invalid srcAddress")


revert, not necessary

dustinxie · 2022-06-02T17:10:01Z

action/const.go

+ return errors.Errorf("unknown encoding type %d", enc)
+ }
+)
+


not very necessary, seems an overkill

as we discussed, we don't really need this

dustinxie · 2022-06-02T17:12:46Z

action/sealedenvelope.go

-}
-
-// calcHash returns the hash value of SealedEnvelope
-func (sealed *SealedEnvelope) calcHash() (hash.Hash256, error) {


keep this, it is clear to have an internal func (which might be used)

dustinxie · 2022-06-02T17:16:08Z

action/sealedenvelope.go

- return nil
+ sealed.encoding = pbAct.GetEncoding()
+ _, err = sealed.Hash()
+ return err


revert, like we discussed, there's no need to change loadProto at all

dustinxie · 2022-06-02T17:17:02Z

action/sealedenvelope_test.go

+ tsf2, ok2 := validated.Envelope.Action().(*Transfer)
+ NewWithT(t).Expect(tsf1).To(Equal(tsf2))
+ NewWithT(t).Expect(ok1 && ok2).To(BeTrue())
+ })


no need to change these test files

dustinxie · 2022-06-02T20:56:50Z

action/sealedenvelope.go

- hv, err := sealed.calcHash()
- sealed.hash = &Hash256Result{hv, err}
+ if sealed.hash != hash.ZeroHash256 || sealed.hashErr != nil {
+ return sealed.hash, sealed.hashErr


just like the 1st commit

if sealed.hash == hash.ZeroHash256 && sealed.hashErr == nil { sealed.hash, sealed.hashErr = sealed.calcHash() } return sealed.hash, sealed.hashErr

dustinxie · 2022-06-03T16:56:38Z

action/sealedenvelope.go

 }
- return rlpSignedHash(tx, sealed.evmNetworkID, sealed.Signature())
+ sealed.evmNetworkID = config.EVMNetworkID()


why do this? the correct place to set evmNetworkID is in LoadProto()

dustinxie · 2022-06-03T16:57:35Z

action/sealedenvelope.go

 }
+ return sealed.hash, sealed.hashErr


as we discussed, please revert unnecessary changes, and make this an internal func calcHash()

dustinxie · 2022-06-03T16:59:06Z

action/sealedenvelope_test.go

+ se.encoding = v.encoding
+ se.signature = v.sig
+ req.Equal(se2.LoadProto(se.Proto()).Error(), v.err.Error())
+ })


as we discussed, please revert unnecessary changes, do minimum changes to make the test pass

dustinxie · 2022-06-03T17:22:54Z

action/sealedenvelope.go

+ return sealed.hash, sealed.hashErr
+}
+
+func (sealed *SealedEnvelope) calcHash() {


one final comment:

func (sealed *SealedEnvelope) calcHash() (hash.Hash256, error)

then don't need to change this func at all, in the above, just

sealed.hash, sealed.hashErr = calcHash()

codecov · 2022-06-03T20:56:16Z

Codecov Report

Merging #3420 (9e4cec1) into master (7d66698) will increase coverage by 0.00%.
The diff coverage is 92.30%.

@@           Coverage Diff           @@
##           master    #3420   +/-   ##
=======================================
  Coverage   75.16%   75.16%           
=======================================
  Files         236      236           
  Lines       22070    22083   +13     
=======================================
+ Hits        16589    16599   +10     
- Misses       4591     4595    +4     
+ Partials      890      889    -1

Impacted Files	Coverage Δ
action/sealedenvelope.go	`86.31% <92.30%> (+0.94%)`	⬆️
consensus/scheme/rolldpos/rolldposctx.go	`73.59% <0.00%> (-0.49%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d66698...9e4cec1. Read the comment docs.

Liuhaai · 2022-06-03T22:58:26Z

action/const.go

@@ -6,7 +6,9 @@

 package action

-import "github.com/pkg/errors"
+import (


Liuhaai · 2022-06-03T22:58:42Z

action/sealedenvelope.go

@@ -5,12 +5,13 @@ import (

 "github.com/iotexproject/go-pkgs/crypto"
 "github.com/iotexproject/go-pkgs/hash"
+ "github.com/iotexproject/iotex-address/address"


Liuhaai · 2022-06-03T22:58:58Z

action/sealedenvelope.go

@@ -22,6 +23,9 @@ type SealedEnvelope struct {
 evmNetworkID uint32
 srcPubkey crypto.PublicKey
 signature []byte
+ srcAddress address.Address
+ hash hash.Hash256
+ hashErr error


CoderZhi · 2022-06-07T18:36:54Z

action/action_deserializer_test.go

 se.signature = _validSig
+ se.hash = hash.ZeroHash256
+ se.Hash()


any validation for the return values?

CoderZhi · 2022-06-08T02:51:31Z

action/action_deserializer_test.go

 se1, err := (&Deserializer{}).ActionToSealedEnvelope(se.Proto())
+ se1.Hash()


Liuhaai · 2022-06-08T05:17:26Z

actpool/actpool_test.go

+ _, err = validated[i].Hash()
+ require.NoError(err)
+ }
+ require.Equal(validated, acts)


asserting hash

* remove ActionsByBlock() in coreService

saitofun requested a review from a team as a code owner May 30, 2022 04:59

dustinxie reviewed May 30, 2022

View reviewed changes

CoderZhi reviewed May 31, 2022

View reviewed changes

saitofun requested review from dustinxie, CoderZhi and Liuhaai June 2, 2022 14:54

dustinxie reviewed Jun 2, 2022

View reviewed changes

saitofun requested a review from dustinxie June 3, 2022 07:10

dustinxie reviewed Jun 3, 2022

View reviewed changes

saitofun requested a review from dustinxie June 3, 2022 17:17

dustinxie reviewed Jun 3, 2022

View reviewed changes

saitofun requested a review from dustinxie June 3, 2022 18:04

saitofun force-pushed the issue2989 branch 2 times, most recently from b10ff27 to c41afb2 Compare June 3, 2022 20:39

saitofun force-pushed the issue2989 branch 2 times, most recently from cd34ff4 to c7a4bfe Compare June 3, 2022 20:45

dustinxie approved these changes Jun 3, 2022

View reviewed changes

dustinxie force-pushed the issue2989 branch from 9297d7a to 1e3df3d Compare June 3, 2022 21:53

Liuhaai reviewed Jun 3, 2022

View reviewed changes

saitofun requested a review from Liuhaai June 6, 2022 00:39

saitofun and others added 5 commits June 7, 2022 13:12

[action] add address and hash field in SealedEnvelope

b9bf8a4

Update sealedenvelope_test.go

4dde2a1

update const.go

880dd09

update sealedenvelope.go

86f2613

remove SealedEnvelope.hashErr

2ef4ce5

saitofun force-pushed the issue2989 branch from 1e52c8d to 2ef4ce5 Compare June 7, 2022 05:12

CoderZhi reviewed Jun 8, 2022

View reviewed changes

Liuhaai approved these changes Jun 8, 2022

View reviewed changes

Liuhaai reviewed Jun 8, 2022

View reviewed changes

actpool/actpool_test.go Outdated

_, err = validated[i].Hash()

require.NoError(err)

}

require.Equal(validated, acts)

Copy link

Member

Liuhaai Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asserting hash

saitofun and others added 5 commits June 9, 2022 09:11

Update actpool_test.go and blockdao_test.go asserting hash

92b27ef

[api] Add mutex to chainListener (#3433)

bab5af5

[api] Remove ActionsByBlock() in coreService (#3432)

8ac5045

* remove ActionsByBlock() in coreService

[protocol] remove config.EVMNetworkID() from EVM/execution (#3431)

ab0de5d

Merge branch 'master' into issue2989

9e4cec1

saitofun merged commit a222431 into master Jun 9, 2022

saitofun deleted the issue2989 branch June 9, 2022 01:39

		se1, err := (&Deserializer{}).ActionToSealedEnvelope(se.Proto())
		se1.Hash()

[action] add address and hash field in SealedEnvelope #3420

[action] add address and hash field in SealedEnvelope #3420

Conversation

saitofun commented May 30, 2022 • edited Loading

Description

Type of change

Checklist:

CLAassistant commented May 30, 2022 • edited Loading

dustinxie May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 3, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[action] add `address` and `hash` field in SealedEnvelope #3420

[action] add `address` and `hash` field in SealedEnvelope #3420

saitofun commented May 30, 2022 •

edited

Loading

CLAassistant commented May 30, 2022 •

edited

Loading

dustinxie May 30, 2022 •

edited

Loading

dustinxie May 30, 2022 •

edited

Loading

dustinxie Jun 3, 2022 •

edited

Loading

dustinxie Jun 2, 2022 •

edited

Loading

codecov bot commented Jun 3, 2022 •

edited

Loading