Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "send enabled" on marker removal and in bulk on 1.10.0 upgrade #821

Merged

Conversation

derekadams
Copy link
Contributor

Description

Remove the "send enabled" bank denom metadata in cases where a marker is deleted. Also add an upgrade handler (lava) that iterates through the "send enabled" metadata and removes it for any that is not associated with a marker.

closes: #819


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • 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
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #821 (9142472) into main (a64b4ea) will increase coverage by 0.04%.
The diff coverage is 85.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #821      +/-   ##
==========================================
+ Coverage   56.14%   56.19%   +0.04%     
==========================================
  Files         179      179              
  Lines       21816    21850      +34     
==========================================
+ Hits        12248    12278      +30     
- Misses       8648     8652       +4     
  Partials      920      920              
Impacted Files Coverage Δ
app/upgrades.go 7.57% <0.00%> (-0.30%) ⬇️
x/marker/keeper/keeper.go 84.48% <100.00%> (+0.27%) ⬆️
x/marker/keeper/marker.go 54.41% <100.00%> (+2.31%) ⬆️

@Taztingo
Copy link
Contributor

It looks like linting is failing. Think you just have to run make lint

Here is the full documentation on it...

Please make sure to run make format before every commit - the easiest way to do this is have your editor run it for you upon saving a file. Additionally please ensure that your code is lint compliant by running make lint. A convenience git pre-commit hook that runs the formatters automatically before each commit is available in the contrib/githooks/ directory.

@channa-figure
Copy link
Contributor

Is it possible to have a test for this? Or is that not needed?

iramiller
iramiller previously approved these changes May 11, 2022
Copy link
Member

@iramiller iramiller left a comment

Choose a reason for hiding this comment

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

LGTM

app/upgrades.go Outdated Show resolved Hide resolved
@iramiller
Copy link
Member

This could stand to have a simple test added for the removeSendEnabled flow...

@channa-figure
Copy link
Contributor

Agree with @iramiller could add the keeper test. I don't think we have a test flow for the upgrade handlers.

channa-figure
channa-figure previously approved these changes May 11, 2022
Copy link
Contributor

@channa-figure channa-figure left a comment

Choose a reason for hiding this comment

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

Thanks for the test. LGTM.

channa-figure
channa-figure previously approved these changes May 11, 2022
@derekadams derekadams enabled auto-merge May 11, 2022 20:24
@iramiller
Copy link
Member

Test should have a denom that is valid (hash?) which is not removed ... that would show both code paths ... otherwise looks good.

@derekadams derekadams changed the title Remove "send enabled" on marker removal and in bulk on 1.9.1 upgrade Remove "send enabled" on marker removal and in bulk on 1.10.0 upgrade May 11, 2022
@channa-figure channa-figure self-requested a review May 11, 2022 22:25
@derekadams derekadams merged commit 29be591 into main May 11, 2022
@derekadams derekadams deleted the dadams/819-remove-send-enabled-status-on-marker-delete branch May 11, 2022 22:25
@iramiller iramiller added this to the v1.10.0 milestone Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updated marker deletion process to remove restricted config
4 participants