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

chore(feemarket) - Depreacte usage of x/params in x/feemarket #1509

Merged
merged 31 commits into from
Jan 5, 2023

Conversation

Vvaradinov
Copy link
Contributor

Deprecates the now legacy Cosmos SDK x/params usage in x/feemarket module. For more info regarding changes check out #1472

Closes: ENG-1107

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)

@Vvaradinov Vvaradinov self-assigned this Nov 29, 2022
@linear
Copy link

linear bot commented Nov 29, 2022

@github-actions github-actions bot added C:Proto protobuf files (*.pb.go) Type: Build codebase build process labels Nov 29, 2022
rpc/backend/chain_info.go Fixed Show fixed Hide fixed
rpc/backend/chain_info.go Fixed Show fixed Hide fixed
@github-actions github-actions bot removed the Type: Build codebase build process label Nov 29, 2022
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #1509 (e7fd485) into main (8886ce3) will decrease coverage by 0.20%.
The diff coverage is 67.85%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1509      +/-   ##
==========================================
- Coverage   68.40%   68.19%   -0.21%     
==========================================
  Files         106      110       +4     
  Lines       10077    10087      +10     
==========================================
- Hits         6893     6879      -14     
- Misses       2783     2801      +18     
- Partials      401      407       +6     
Impacted Files Coverage Δ
x/feemarket/types/codec.go 45.45% <45.45%> (ø)
x/feemarket/keeper/keeper.go 86.66% <50.00%> (-4.64%) ⬇️
x/feemarket/types/msg.go 50.00% <50.00%> (ø)
x/feemarket/keeper/params.go 68.42% <63.63%> (-20.87%) ⬇️
x/feemarket/keeper/msg_server.go 66.66% <66.66%> (ø)
x/feemarket/migrations/v4/migrate.go 83.33% <83.33%> (ø)
app/app.go 84.90% <100.00%> (+0.02%) ⬆️
x/feemarket/keeper/migrations.go 100.00% <100.00%> (+100.00%) ⬆️
x/feemarket/types/params.go 87.69% <100.00%> (-5.42%) ⬇️
... and 2 more

@Vvaradinov Vvaradinov marked this pull request as ready for review November 29, 2022 21:41
@Vvaradinov Vvaradinov requested a review from a team as a code owner November 29, 2022 21:41
@Vvaradinov Vvaradinov requested review from hanchon and removed request for a team November 29, 2022 21:41
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.

Awesome work @Vvaradinov!! Left a few questions and comments, LMK your thoughts

proto/ethermint/feemarket/v1/tx.proto Outdated Show resolved Hide resolved
proto/ethermint/feemarket/v1/tx.proto Outdated Show resolved Hide resolved
x/feemarket/genesis.go Outdated Show resolved Hide resolved
x/feemarket/genesis.go Outdated Show resolved Hide resolved
proto/ethermint/feemarket/v1/tx.proto Outdated Show resolved Hide resolved
x/feemarket/keeper/msg_server.go Outdated Show resolved Hide resolved
x/feemarket/migrations/v4/migrate.go Outdated Show resolved Hide resolved
x/feemarket/module.go Outdated Show resolved Hide resolved
x/feemarket/types/msg.go Outdated Show resolved Hide resolved
x/feemarket/types/msg.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.

tACK! Good stuff @Vvaradinov, left some small comments

proto/ethermint/feemarket/v1/feemarket.proto Outdated Show resolved Hide resolved
x/feemarket/keeper/keeper.go Show resolved Hide resolved
x/feemarket/keeper/msg_server.go Outdated Show resolved Hide resolved
x/feemarket/keeper/params.go Outdated Show resolved Hide resolved
x/feemarket/keeper/params.go Outdated Show resolved Hide resolved
x/feemarket/types/codec.go Outdated Show resolved Hide resolved
x/feemarket/types/params.go Show resolved Hide resolved
proto/ethermint/feemarket/v1/tx.proto Outdated Show resolved Hide resolved
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. We should improve coverage in the msg.go file

x/feemarket/keeper/params.go Outdated Show resolved Hide resolved
Co-authored-by: Federico Kunze Küllmer <[email protected]>
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.

@Vvaradinov requesting some additional changes that we discussed in Slack

x/feemarket/keeper/keeper.go Show resolved Hide resolved
x/feemarket/keeper/msg_server.go Show resolved Hide resolved
x/feemarket/keeper/params.go Show resolved Hide resolved
x/feemarket/types/msg.go Show resolved Hide resolved
x/feemarket/types/msg.go Outdated Show resolved Hide resolved
x/feemarket/keeper/params.go Outdated Show resolved Hide resolved
@fedekunze fedekunze enabled auto-merge (squash) January 5, 2023 16:29
@fedekunze fedekunze merged commit 57ed355 into main Jan 5, 2023
@fedekunze fedekunze deleted the Vvaradinov/refactor-feemarket-params branch January 5, 2023 16:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C:Proto protobuf files (*.pb.go)
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants