Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

Use value copy instead of pointer copy in stateObject.deepCopy #741

Closed
wants to merge 7 commits into from

Conversation

summerpro
Copy link
Contributor

@summerpro summerpro commented Jan 21, 2021

Closes: #740

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Comment on lines 403 to 411
newAccount := ethermint.ProtoAccount().(*ethermint.EthAccount)
jsonAccount, err := so.account.MarshalJSON()
if err != nil {
return nil
}
err = newAccount.UnmarshalJSON(jsonAccount)
if err != nil {
return nil
}
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

@summerpro summerpro requested a review from fedekunze February 1, 2021 02:42
newAccount := ethermint.ProtoAccount().(*ethermint.EthAccount)
jsonAccount, err := so.account.MarshalJSON()
if err != nil {
return nil
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@summerpro summerpro Feb 5, 2021

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.

Copy link
Contributor Author

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

@summerpro summerpro requested a review from freddyli7 February 7, 2021 02:33
@github-actions
Copy link

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.

@github-actions github-actions bot added the stale stale PRs that will be closed if no further action occurs label Mar 26, 2021
@github-actions github-actions bot closed this Apr 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
stale stale PRs that will be closed if no further action occurs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "deepCopy" function from "StateObject" makes pointer copy instead of value copy
3 participants