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

refactor: Adding improvements from the official Gravity Bridge #351

Closed
wants to merge 21 commits into from

Conversation

facundomedica
Copy link
Contributor

@facundomedica facundomedica commented Dec 30, 2021

Description

In this PR I'm pulling the latest updates made on the official GB to our Peggy module. There'll be more PRs like this in the future.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added appropriate labels to the PR
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@facundomedica facundomedica changed the title ref: Refactor adding improvements from the official Gravity Bridge refactor: Adding improvements from the official Gravity Bridge Dec 30, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 30, 2021

Codecov Report

Merging #351 (dec3173) into main (2bcfeb3) will increase coverage by 0.09%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
+ Coverage   37.16%   37.25%   +0.09%     
==========================================
  Files          78       78              
  Lines       10858    10940      +82     
==========================================
+ Hits         4035     4076      +41     
- Misses       6518     6552      +34     
- Partials      305      312       +7     
Impacted Files Coverage Δ
x/peggy/keeper/attestation_handler.go 3.14% <0.00%> (-0.74%) ⬇️
x/peggy/keeper/batch.go 64.03% <0.00%> (ø)
x/peggy/keeper/keeper.go 53.41% <57.89%> (+1.67%) ⬆️
x/peggy/abci.go 89.54% <80.85%> (-3.87%) ⬇️

@facundomedica
Copy link
Contributor Author

@alexanderbez this is the part I'm unsure about:

umee/x/peggy/abci.go

Lines 277 to 281 in 4f8a32d

h.k.StakingKeeper.Slash(ctx, valConsAddr, ctx.BlockHeight(), consPower, params.SlashFractionValset)
if !validator.IsJailed() {
h.k.StakingKeeper.Jail(ctx, valConsAddr)
}

I think it's ok to slash a jailed validator in this case. But, will that get the same validator slashed on every endBlock?

@facundomedica facundomedica marked this pull request as ready for review January 3, 2022 17:17
x/peggy/abci.go Outdated Show resolved Hide resolved
x/peggy/abci.go Outdated Show resolved Hide resolved
x/peggy/abci.go Outdated Show resolved Hide resolved
x/peggy/abci.go Outdated Show resolved Hide resolved
x/peggy/keeper/keeper.go Outdated Show resolved Hide resolved
x/peggy/abci.go Show resolved Hide resolved
x/peggy/abci.go Outdated Show resolved Hide resolved
x/peggy/abci.go Outdated Show resolved Hide resolved
consPower,
params.SlashFractionValset,
)
ctx.EventManager().EmitEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

see comments below on events 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This event is still wrong btw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexanderbez pls check now

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Looks good. WDYT about valset_slashing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case we will need valset_slashing and batch_slashing. Idk how much an event type should englobe, or if it should be very narrow

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be specific to the domain, in this case, valset slashing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we need to update events.md, which probably could use an entire overhaul, to reflect these new events.

I don't have a strong opinion on slashing vs valset_slashing, I just wanted to suggest it.

h.k.StakingKeeper.Slash(ctx, valConsAddr, ctx.BlockHeight(), consPower, params.SlashFractionValset)
ctx.EventManager().EmitEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

see comments below on events 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

x/peggy/abci.go Outdated Show resolved Hide resolved
x/peggy/abci.go Outdated Show resolved Hide resolved
consPower,
params.SlashFractionValset,
)
ctx.EventManager().EmitEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

This event is still wrong btw

h.k.StakingKeeper.Slash(ctx, valConsAddr, ctx.BlockHeight(), consPower, params.SlashFractionValset)
ctx.EventManager().EmitEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

x/peggy/abci.go Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

@facundomedica can we close this?

@facundomedica
Copy link
Contributor Author

@alexanderbez thanks for the reminder. Closed!

@facundomedica facundomedica deleted the facu/fix-slashing-logic branch January 6, 2022 22:15
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.

3 participants