Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

fix: coinbase should not be the current one in traceTransaction execution #1392

Merged
merged 14 commits into from
Oct 21, 2022
Merged

fix: coinbase should not be the current one in traceTransaction execution #1392

merged 14 commits into from
Oct 21, 2022

Conversation

mmsqe
Copy link
Contributor

@mmsqe mmsqe commented Oct 20, 2022

Closes: #1391

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)

@mmsqe mmsqe changed the title Problem: coinbase should not be the current one in traceTransaction execution fix: coinbase should not be the current one in traceTransaction execution Oct 20, 2022
@codecov
Copy link

codecov bot commented Oct 20, 2022 •
edited
Loading

Codecov Report

Merging #1392 (748e056) into main (f04b289) will decrease coverage by 0.06%.
The diff coverage is 37.20%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1392      +/-   ##
==========================================
- Coverage   57.66%   57.59%   -0.07%     
==========================================
  Files         108      108              
  Lines       10074    10087      +13     
==========================================
+ Hits         5809     5810       +1     
- Misses       4019     4031      +12     
  Partials      246      246
Impacted Files Coverage Δ
rpc/backend/call_tx.go 0.00% <0.00%> (ø)
rpc/backend/tracing.go 0.00% <0.00%> (ø)
x/evm/keeper/grpc_query.go 89.13% <100.00%> (-0.08%) ⬇️
x/evm/keeper/state_transition.go 82.75% <100.00%> (+0.20%) ⬆️

@mmsqe mmsqe marked this pull request as ready for review October 20, 2022 07:22
@mmsqe mmsqe requested a review from a team as a code owner October 20, 2022 07:22
@mmsqe mmsqe requested review from GAtom22 and 4rgon4ut and removed request for a team October 20, 2022 07:22
facs95
facs95 suggested changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@facs95 facs95 left a comment

Choose a reason for hiding this comment

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

Overall looks good! Thanks for adding this. You still have to include the ProposerAddress on the request from the RPC request eth_traceTransaction here

yihuang
yihuang reviewed Oct 21, 2022
View reviewed changes
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
yihuang
yihuang approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

LGTM

fedekunze
fedekunze reviewed Oct 21, 2022
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM. Minor suggestion

rpc/backend/call_tx.go Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/query.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/query.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/query.proto Outdated Show resolved Hide resolved
@fedekunze fedekunze requested a review from facs95 October 21, 2022 20:53
@fedekunze fedekunze enabled auto-merge (squash) October 21, 2022 20:53
@fedekunze fedekunze merged commit 295a886 into evmos:main Oct 21, 2022
@danburck danburck mentioned this pull request Nov 30, 2022
Closed
mmsqe added a commit to mmsqe/ethermint that referenced this pull request Jan 16, 2023
@mmsqe @facs95 @fedekunze
fix(evm,rpc): coinbase should not be the current one in traceTransact…
10bba9d 
…ion execution (evmos#1392)

* add proposer address

* make proto-all

* update nix

* fix test

* keep default proposerAddress

* add change doc

* refine GetProposerAddress with test

* include ProposerAddress for trace api

* fix eth call req

* wrap proposerAddress for eth call

* allow proto translates to sdk.ConsAddress

* Update rpc/backend/call_tx.go

Co-authored-by: Freddy Caceres <facs95@gmail.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@zsystm zsystm mentioned this pull request Dec 8, 2023
Merged
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@4rgon4ut 4rgon4ut 4rgon4ut left review comments

@fedekunze fedekunze fedekunze approved these changes

@facs95 facs95 facs95 approved these changes

@yihuang yihuang yihuang approved these changes

@GAtom22 GAtom22 Awaiting requested review from GAtom22 GAtom22 is a code owner automatically assigned from evmos/core-engineering

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

Successfully merging this pull request may close these issues.

Problem: coinbase should not be the current one in traceTransaction execution
5 participants
@mmsqe @yihuang @facs95 @fedekunze @4rgon4ut