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

Update web3 transaction hash from RLP #250

Merged
merged 2 commits into from
Apr 13, 2020
Merged

Update web3 transaction hash from RLP #250

merged 2 commits into from
Apr 13, 2020

Conversation

austinabell
Copy link

Closes: #XXX

Description

  • Updates the prepare of the statedb to properly use the amino hash (was using the RLP hash for the web3 transaction type.
  • Removes the hash function from that type so no confusion or misuse in the future (don't think the function can just be changed to amino hash and cache that because the tx encoder isn't known at this function)
    • I don't think this causes any problems, and the reason I was apprehensive before changing this is because signatures are derived by the RLP hash. This function was not used for any signature verification, so I assume everything will still work (Be careful though because it still uses RLP to sign and recover)
  • Tx hash in the web3 api response is now correctly indicating the correct hash to index the transaction, instead of returning the Ethereum hash (might be worth looking more into to make sure dev tooling does not break because of this)

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)

fedekunze
fedekunze approved these changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment •
edited
Loading

Choose a reason for hiding this comment

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

tested ACK. 1 comment. Can you add a changelog entry under bug fixes?

x/evm/handler.go
// Prepare db for logs
// TODO: block hash
k.CommitStateDB.Prepare(txHash, txHash, k.TxCount)
k.CommitStateDB.Prepare(ethHash, common.Hash{}, k.TxCount)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove the second field from the Prepare function signature if it's not going to be used?

Copy link
Author

Choose a reason for hiding this comment

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

it's matching the Geth interface, and will be used in the future (for adding block hash for the transactions logs)

letterj
letterj approved these changes Apr 13, 2020
View reviewed changes
Copy link

@letterj letterj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

Both ethermint-scripts and ethermint-deploy ran successfully

@fedekunze fedekunze merged commit 33ab63e into development Apr 13, 2020
@fedekunze fedekunze deleted the austin/removeethhash branch April 13, 2020 23:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fedekunze fedekunze fedekunze approved these changes

@letterj letterj letterj approved these changes

@noot noot Awaiting requested review from noot

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Strange hash in transaction obj
3 participants
@austinabell @letterj @fedekunze