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

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

Merged
fedekunze merged 23 commits into from
Jan 5, 2023
Merged

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

fedekunze merged 23 commits into from
Jan 5, 2023

Conversation

@Vvaradinov
Copy link
Contributor

@Vvaradinov Vvaradinov commented Dec 8, 2022

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 MalteHerrmann and austinchandra 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 •
edited
Loading

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
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 8, 2022
View reviewed changes
GAtom22
GAtom22 approved these changes Dec 12, 2022
View reviewed changes
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!

ramacarlucho
ramacarlucho approved these changes Dec 12, 2022
View reviewed changes
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

fedekunze
fedekunze approved these changes Dec 15, 2022
View reviewed changes
Copy link
Collaborator

@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

MalteHerrmann
MalteHerrmann reviewed Jan 3, 2023
View reviewed changes
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?

@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
@mmsqe
Revert "chore(evm, feemarket) - Migrate Event emitting to TypedEvent (e…
edc97be 
…vmos#1544)"

This reverts commit 8886ce3.
@mmsqe mmsqe mentioned this pull request Jan 10, 2023
Merged
11 tasks
@yihuang yihuang mentioned this pull request Jan 10, 2023
Closed
@Vvaradinov Vvaradinov mentioned this pull request Jan 11, 2023
Merged
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

4 more reviewers

@MalteHerrmann MalteHerrmann MalteHerrmann left review comments

@ramacarlucho ramacarlucho ramacarlucho approved these changes

@fedekunze fedekunze fedekunze approved these changes

@GAtom22 GAtom22 GAtom22 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Vvaradinov Vvaradinov

Labels

C:Proto protobuf files (*.pb.go) Type: Build codebase build process Type: Tests issues and PR related to tests

Projects

No open projects
Ethermint Board
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Vvaradinov @ramacarlucho @fedekunze @MalteHerrmann @GAtom22