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

MVCC part A #152

Merged
merged 1 commit into from
Jun 20, 2022
Merged

MVCC part A #152

merged 1 commit into from
Jun 20, 2022

Conversation

sainoe
Copy link
Contributor

@sainoe sainoe commented Jun 15, 2022

Partially solve #139 and contain all the changes that can be done before #150 is merged.

Disable the following modules in the consumer:

  • Distribution
  • Governance
  • Mint
  • Genutil
  • Liquidity

Note that TestDistribution in provider_test.go started failing after removing the mint module
and is now commented.

 * Distribution
 * Governance
 * Mint
 * Genutil
 * Liquidity

Note that TestDistribution in provider_test.go was commented and needs to be debugged
and the fee distribution rate handling was also commented in prepForZeroHeightGenesis
"ibc/3C3D7B3BE4ECC85A0E5B52A3AEC3B7DFC2AA9CA47C37821E57020D6807043BE9").RoundInt64()
s.Assert().Equal(balanceI64, expProviderAmt)
}
// func (s *ProviderTestSuite) TestDistribution() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpoke
Could you ping @rigelrozanski to debug this :) ?
I can hop on call with him to help

Copy link
Contributor

Choose a reason for hiding this comment

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

Yo. Okay so the distribution tests are failing because mint has been removed from app/consumer/app.go. If there is nothing to distribute then there will be nothing to test. This test could be modified to artificially add tokens to the FeePool (basically simulating mint, or fees being spent on the consumer chain) but that doesn't seem like a great use of time, the test would need to be largely rewritten. So if the consensus is to remove mint, then I'd recommend adding a bool in the consumer app.go that optionally removes mint so it can still be used for testing. OR alternatively if mint will be re-added at some later point, this test can just remain commented out for the time being.

I guess I don't really understand why mint isn't a part of the MVCC? Isn't the whole idea that a consumer chain has a token which it uses for something? If it doesn't have mint, it doesn't have a token to send back... I guess just the consumer would simply send back any fees collected... which may work I suppose (thinking out loud here).

Copy link
Contributor

Choose a reason for hiding this comment

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

@rigelrozanski We plan to have multiple flavors of consumer chains, each one with its own app.go. For the minimum viable consumer chain (MVCC), the mint module is disable since there is no native token. For the governance-enabled consumer chain (see #141), the mint module is required.

Regarding testing, can't we just add in the unit test some tokens to feeCollectorName account on the consumer chain? Does that require a lot of work?

@sainoe sainoe requested review from tac0turtle and okwme June 15, 2022 12:39
@sainoe sainoe mentioned this pull request Jun 15, 2022
@mpoke mpoke requested a review from rigelrozanski June 16, 2022 10:58
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

Nice job!

Comment on lines -78 to +121
app.StakingKeeper.IterateValidators(ctx, func(_ int64, val stakingtypes.ValidatorI) (stop bool) {
_, err := app.DistrKeeper.WithdrawValidatorCommission(ctx, val.GetOperator())
if err != nil {
panic(err)
}
return false
})
// app.StakingKeeper.IterateValidators(ctx, func(_ int64, val stakingtypes.ValidatorI) (stop bool) {
// _, err := app.DistrKeeper.WithdrawValidatorCommission(ctx, val.GetOperator())
// if err != nil {
// panic(err)
// }
// return false
// })

// withdraw all delegator rewards
dels := app.StakingKeeper.GetAllDelegations(ctx)
for _, delegation := range dels {
_, err := app.DistrKeeper.WithdrawDelegationRewards(ctx, delegation.GetDelegatorAddr(), delegation.GetValidatorAddr())
if err != nil {
panic(err)
}
}
// dels := app.StakingKeeper.GetAllDelegations(ctx)
// for _, delegation := range dels {
// _, err := app.DistrKeeper.WithdrawDelegationRewards(ctx, delegation.GetDelegatorAddr(), delegation.GetValidatorAddr())
// if err != nil {
// panic(err)
// }
// }

// clear validator slash events
app.DistrKeeper.DeleteAllValidatorSlashEvents(ctx)
// app.DistrKeeper.DeleteAllValidatorSlashEvents(ctx)

// clear validator historical rewards
app.DistrKeeper.DeleteAllValidatorHistoricalRewards(ctx)
// app.DistrKeeper.DeleteAllValidatorHistoricalRewards(ctx)

// set context height to zero
height := ctx.BlockHeight()
ctx = ctx.WithBlockHeight(0)

// reinitialize all validators
app.StakingKeeper.IterateValidators(ctx, func(_ int64, val stakingtypes.ValidatorI) (stop bool) {
// donate any unwithdrawn outstanding reward fraction tokens to the community pool
scraps := app.DistrKeeper.GetValidatorOutstandingRewardsCoins(ctx, val.GetOperator())
feePool := app.DistrKeeper.GetFeePool(ctx)
feePool.CommunityPool = feePool.CommunityPool.Add(scraps...)
app.DistrKeeper.SetFeePool(ctx, feePool)

app.DistrKeeper.Hooks().AfterValidatorCreated(ctx, val.GetOperator())
return false
})
// app.StakingKeeper.IterateValidators(ctx, func(_ int64, val stakingtypes.ValidatorI) (stop bool) {
// // donate any unwithdrawn outstanding reward fraction tokens to the community pool
// scraps := app.DistrKeeper.GetValidatorOutstandingRewardsCoins(ctx, val.GetOperator())
// feePool := app.DistrKeeper.GetFeePool(ctx)
// feePool.CommunityPool = feePool.CommunityPool.Add(scraps...)
// app.DistrKeeper.SetFeePool(ctx, feePool)

// app.DistrKeeper.Hooks().AfterValidatorCreated(ctx, val.GetOperator())
// return false
// })

// reinitialize all delegations
for _, del := range dels {
app.DistrKeeper.Hooks().BeforeDelegationCreated(ctx, del.GetDelegatorAddr(), del.GetValidatorAddr())
app.DistrKeeper.Hooks().AfterDelegationModified(ctx, del.GetDelegatorAddr(), del.GetValidatorAddr())
}
// for _, del := range dels {
// app.DistrKeeper.Hooks().BeforeDelegationCreated(ctx, del.GetDelegatorAddr(), del.GetValidatorAddr())
// app.DistrKeeper.Hooks().AfterDelegationModified(ctx, del.GetDelegatorAddr(), del.GetValidatorAddr())
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not fan of commenting out code, it might be nicer to just delete it and put a comment with a permalink to the commit with the deleted code. It's just my preference. Ultimately it's up to you

Copy link
Contributor

Choose a reason for hiding this comment

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

I usually leave commented code in until the system is pretty much finalized, then delete code all the code still commented at that point

Comment on lines +90 to +93
// set chains sender account number
// TODO: to be fixed in #151
path.EndpointB.Chain.SenderAccount.SetAccountNumber(6)
path.EndpointA.Chain.SenderAccount.SetAccountNumber(6)
path.EndpointA.Chain.SenderAccount.SetAccountNumber(3)
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work @sainoe. Let's open another issue to deal with TestDistribution.

@sainoe sainoe merged commit 5ee03ef into main Jun 20, 2022
@sainoe sainoe deleted the sainoe/mvcc-disable-modules-A branch June 20, 2022 17:06
@sainoe sainoe mentioned this pull request Jun 20, 2022
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.

4 participants