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

candidate fix for v18 #6144

Merged
merged 15 commits into from
Aug 24, 2023
Merged

candidate fix for v18 #6144

merged 15 commits into from
Aug 24, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Aug 24, 2023

Closes: #XXX

What is the purpose of the change

Needs to be cherry-picked onto main

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added unit test that validates ...
  • Added integration tests for end-to-end deployment with ...
  • Extended integration test for ...
  • Manually verified the change by ...

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

app/upgrades/v18/upgrades_test.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades_test.go Outdated Show resolved Hide resolved
Comment on lines +361 to +375
// In this case, remove redundant cases.
// Namely: gauge empty OR gauge coins undistributable.
if remainCoins.Empty() {
ctx.Logger().Debug(fmt.Sprintf("gauge debug, this gauge is empty, why is it being ran %d. Balancer code", gauge.Id))
err := k.updateGaugePostDistribute(ctx, gauge, totalDistrCoins)
return totalDistrCoins, err
}

// Remove some spam gauges, is state compatible.
// If they're to pool 1 they can't distr at this small of a quantity.
if remainCoins.Len() == 1 && remainCoins[0].Amount.LTE(sdk.NewInt(10)) && gauge.DistributeTo.Denom == "gamm/pool/1" && remainCoins[0].Denom != "uosmo" {
ctx.Logger().Debug(fmt.Sprintf("gauge debug, this gauge is perceived spam, skipping %d", gauge.Id))
err := k.updateGaugePostDistribute(ctx, gauge, totalDistrCoins)
return totalDistrCoins, err
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the state-compatible fix to speed up incentive distribution. Makes sense to me to include it

@@ -295,6 +295,22 @@ func (k Keeper) clearKeysByPrefix(ctx sdk.Context, prefix []byte) {
}
}

func (k Keeper) RebuildAccumulationStoreForDenom(ctx sdk.Context, denom string) {
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: unit test this if time

p0mvn and others added 6 commits August 24, 2023 11:24
* fix balancer liquidity breaking incentives

* clean up

* comment

* added swap checks

* check that LPing fails before the upgrade

* fix test

---------

Co-authored-by: Nicolas Lara <nicolaslara@gmail.com>
@ValarDragon ValarDragon marked this pull request as ready for review August 24, 2023 14:10
@@ -330,7 +330,7 @@ func (k Keeper) distributeInternal(
// Note: reason why we do millisecond conversion is because floats are non-deterministic.
emissionRate := sdk.NewDecFromInt(remainAmountPerEpoch).QuoTruncate(sdk.NewDec(currentEpoch.Duration.Milliseconds()).QuoInt(sdk.NewInt(1000)))

ctx.Logger().Debug("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())
ctx.Logger().Info("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())
Copy link
Member

@ValarDragon ValarDragon Aug 24, 2023

Choose a reason for hiding this comment

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

This got converted to Info, it should be debug

@@ -330,7 +330,7 @@ func (k Keeper) distributeInternal(
// Note: reason why we do millisecond conversion is because floats are non-deterministic.
emissionRate := sdk.NewDecFromInt(remainAmountPerEpoch).QuoTruncate(sdk.NewDec(currentEpoch.Duration.Milliseconds()).QuoInt(sdk.NewInt(1000)))

ctx.Logger().Debug("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())
ctx.Logger().Info("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ctx.Logger().Info("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())
ctx.Logger().Debug("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())

@@ -66,7 +66,8 @@ func (k Keeper) RefreshIntermediaryDelegationAmounts(ctx sdk.Context) {
if !found {
// continue if current delegation is 0, in case its really a dust delegation
// that becomes worth something after refresh.
k.Logger(ctx).Info(fmt.Sprintf("Existing delegation not found for %s with %s during superfluid refresh."+
// TODO: We have a correct explanation for this in some github issue, lets amend this correctly.
k.Logger(ctx).Debug(fmt.Sprintf("Existing delegation not found for %s with %s during superfluid refresh."+
Copy link
Member

Choose a reason for hiding this comment

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

Spares our epoch logs 🙏

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Tested this on a flow where we:
Make a v18 with broken accum store
Make a v19 with these changes

We saw that v18 caused the break as expected, and v19 fixed it.

Hence this as v19 should fix the mainnet problem.

The load bearing part of the fix is actually ran every testnet as InitGenesis

@ValarDragon ValarDragon merged commit 1c5f25d into v18.x Aug 24, 2023
1 check passed
@ValarDragon ValarDragon deleted the dev/v18.x_fix branch August 24, 2023 14:20
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants