-
Notifications
You must be signed in to change notification settings - Fork 160
Use value copy instead of pointer copy in stateObject.deepCopy #741
Conversation
x/evm/types/state_object.go
Outdated
newAccount := ethermint.ProtoAccount().(*ethermint.EthAccount) | ||
jsonAccount, err := so.account.MarshalJSON() | ||
if err != nil { | ||
return nil | ||
} | ||
err = newAccount.UnmarshalJSON(jsonAccount) | ||
if err != nil { | ||
return nil | ||
} |
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.
what is this for? why don't you copy the existing fields to a new EthAccount
?
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.
The member variable PubKey of ETHAccount is of interface type, do you have a better suggestion for deep copy of interface type?
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.
@fedekunze Hi, what do you think of this solution? Can it be passed? Are there any possible problems with this solution?
x/evm/types/state_object.go
Outdated
newAccount := ethermint.ProtoAccount().(*ethermint.EthAccount) | ||
jsonAccount, err := so.account.MarshalJSON() | ||
if err != nil { | ||
return nil |
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.
seems that using value copy is ok here but when error happens during marshal
or unmarshal
, error will be suppressed and nil will be returned, in this case, any one who invoked deepCopy
may encounter nil pointer dereference
and there is no way to prevent it.
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.
The error returned in marshal is usually due to an operating system error. In this case, panic may be more appropriate.
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 wrote another solution, you can take a look(link), but I am not familiar with the usage of reflect, and I am worried that there will be any problems.
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.
@freddyli7 Looking forward to your reply
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days-before-close if no further activity occurs. Thank you for your contributions. |
Closes: #740
Description
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)