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

chore(evm, feemarket) - Migrate Event emitting to TypedEvent #1544

Merged
merged 23 commits into from
Jan 5, 2023

Conversation

Vvaradinov
Copy link
Contributor

Description

Migrate the usage of now deprecated EmitEvent to EmitTypedEvent. New events.proto is created describing each event type the evm module emits. Marked with a TODO a place where I am unsure how to proceed.

Closes: ENG-1173


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)

@Vvaradinov Vvaradinov requested a review from a team as a code owner December 8, 2022 13:19
@Vvaradinov Vvaradinov requested review from 0a1c and MalteHerrmann and removed request for a team December 8, 2022 13:19
@linear
Copy link

linear bot commented Dec 8, 2022

ENG-1173 Migrate Events - Etheremint

  • evm
  • feemarket

@Vvaradinov Vvaradinov self-assigned this Dec 8, 2022
@github-actions github-actions bot added the C:Proto protobuf files (*.pb.go) label Dec 8, 2022
@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #1544 (1c0cda1) into main (0f7bdce) will decrease coverage by 0.11%.
The diff coverage is 67.44%.

❗ Current head 1c0cda1 differs from pull request most recent head 9ce6547. Consider uploading reports for the commit 9ce6547 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1544      +/-   ##
==========================================
- Coverage   68.52%   68.40%   -0.12%     
==========================================
  Files         106      106              
  Lines       10077    10077              
==========================================
- Hits         6905     6893      -12     
- Misses       2775     2783       +8     
- Partials      397      401       +4     
Impacted Files Coverage Δ
x/evm/keeper/state_transition.go 78.29% <0.00%> (ø)
x/feemarket/keeper/abci.go 80.00% <50.00%> (-13.03%) ⬇️
x/evm/keeper/keeper.go 85.54% <57.14%> (-1.59%) ⬇️
x/evm/keeper/msg_server.go 87.36% <82.60%> (-3.55%) ⬇️

@Vvaradinov Vvaradinov changed the title chore(evm) - Migrate Event emitting to TypedEvent chore(evm, feemarket) - Migrate Event emitting to TypedEvent Dec 8, 2022
x/feemarket/keeper/abci.go Fixed Show resolved Hide resolved
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

Great work @Vvaradinov! LGTM!

Copy link
Contributor

@ramacarlucho ramacarlucho left a comment

Choose a reason for hiding this comment

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

pending linter

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.

ACK minor comments

x/evm/keeper/keeper.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

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

basically LGTM, left some minor nit-picking on some comments but that's personal preference really.

@Vvaradinov there are some failing tests though, which seem to reference some of the affected events (e.g. block bloom event is not found in this run) - can you either verify that this is not caused by the changes here or adjust the tests accordingly?

proto/ethermint/evm/v1/events.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/events.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/events.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/events.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/events.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/events.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/events.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/events.proto Outdated Show resolved Hide resolved
proto/ethermint/feemarket/v1/events.proto Outdated Show resolved Hide resolved
proto/ethermint/feemarket/v1/events.proto Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Type: Tests issues and PR related to tests label Jan 3, 2023
@github-actions github-actions bot added the Type: Build codebase build process label Jan 4, 2023
@fedekunze fedekunze enabled auto-merge (squash) January 4, 2023 18:24
@fedekunze fedekunze disabled auto-merge January 5, 2023 16:24
@fedekunze fedekunze merged commit 8886ce3 into main Jan 5, 2023
@fedekunze fedekunze deleted the Vvaradinov/evm-migrate-typed-events branch January 5, 2023 16:24
mmsqe added a commit to mmsqe/ethermint that referenced this pull request Jan 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C:Proto protobuf files (*.pb.go) Type: Build codebase build process Type: Tests issues and PR related to tests
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants