-
Notifications
You must be signed in to change notification settings - Fork 0
feat: ADR-004 (Nakamoto Bonus) #10
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
base: release/v0.50.x
Are you sure you want to change the base?
Conversation
x/distribution/keeper/abci.go
Outdated
c := sdk.UnwrapSDKContext(ctx) | ||
|
||
// dynamically adjust the nakamoto bonus coefficient first. | ||
if err := k.AdjustEta(c); err != nil { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
if ctx.BlockHeight() > 1 { | ||
if err := k.AllocateTokens(ctx, previousTotalPower, ctx.VoteInfos()); err != nil { | ||
if c.BlockHeight() > 1 { | ||
if err := k.AllocateTokens(ctx, previousTotalPower, c.VoteInfos()); err != nil { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
func (am AppModule) BeginBlock(ctx context.Context) error { | ||
c := sdk.UnwrapSDKContext(ctx) | ||
return BeginBlocker(c, am.keeper) | ||
return am.keeper.BeginBlocker(c) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
@Pantani your pull request is missing a changelog! |
Does this PR replace atomone-hub/atomone#182? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! first walk-trough, i'll check the tests more carefully later today/tomorrow
x/distribution/keeper/eta.go
Outdated
return err | ||
} | ||
|
||
if !params.NakamotoBonusEnabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to set it to zero if disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because the parameter is being used inside the allocate tokens. We set it to zero so that we can share the reward among the validades without the Nakamoto bonus
nakamotoCoefficient := params.NakamotoBonusCoefficient // the nakamoto bonus coefficient (e.g., 0.05 means 5% NB, 95% PR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we do that at InitGenesis and/or in a migration instead of here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? the AdjustEta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the enabled check to set the default value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this is the idea, but we can change the value through governance tx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should stay in params, but mean the default value L25
x/distribution/keeper/eta.go
Outdated
|
||
// AdjustEta is called to adjust η dynamically for each block. | ||
func (k Keeper) AdjustEta(ctx sdk.Context) error { | ||
if ctx.BlockHeight()%types.EtaUpdateInterval != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This almost sounds like we could use the x/epochs module for the calculation instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it available in this version? I didn't find it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be compatible yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how I can use it? I couldn't find any reference to this in the SDK. I don't see it in this version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julienrbrt could you give an example to see if the usage of x/epoch
is worthwhile ?
…at/nakamoto-bonus # Conflicts: # Makefile # api/cosmos/distribution/v1beta1/distribution.pulsar.go # x/distribution/types/query.pb.go
13d1ae5
to
f64aa85
Compare
And also atomone-hub/atomone#181 ? If yes please close them @Pantani |
…at/nakamoto-bonus
} | ||
sort.Slice(validators, func(i, j int) bool { | ||
return validators[i].GetBondedTokens().GT(validators[j].GetBondedTokens()) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validators
is already sorted by power, is it different than bonded tokens ?
numValidators := int64(len(bondedVotes)) | ||
nbPerValidator := sdk.NewDecCoinFromDec(bondDenom, math.LegacyZeroDec()) | ||
if numValidators > 0 && len(nakamotoBonus) > 0 { | ||
amount := nakamotoBonus.AmountOf(bondDenom).Quo(math.LegacyNewDec(numValidators)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why limiting to atone
bond denom ?
Currently the inflation rewards contain both atone
and photon
. The Nakamoto bonus should reflect that as well in my opinion.
x/distribution/keeper/eta.go
Outdated
|
||
// AdjustEta is called to adjust η dynamically for each block. | ||
func (k Keeper) AdjustEta(ctx sdk.Context) error { | ||
if ctx.BlockHeight()%types.EtaUpdateInterval != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julienrbrt could you give an example to see if the usage of x/epoch
is worthwhile ?
x/distribution/types/params.go
Outdated
return nil | ||
} | ||
|
||
func validateNakamotoBonusCoefficient(i interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to use an interface{}
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it here because we still use the legacy params table here. And wee need to pass to
func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we still need the legacy params? It looks like everything lives in the keeper.Params
now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda overlook the update of the Nakamoto Coefficient in the initial specs. I wonder if it isn't a bit dangerous to let this coef moving freely between 0 and 1.
Is it really a required feature ? (@giuliostramondo @giunatale)
@tbruyelle we discussed about this in the past and the decision was to let the coefficient move between 0 and 1. What we could do is to allow a max to be set through governance (initially max would be 1 as discussed), what do you think @giunatale ? |
makes sense to me! |
I agree about having a max somewhere in the parameter. As mentionned in my review as well, the coefficient itself shouldn't be a parameter, as it is a moving value. |
IMHO, we could move the coefficient to the store and keep the flag to disable and enable inside the parameters, along with the Nakamoto step |
Yes exactly. You can potentially replace the disable/enable flag with the max parameter. If max=0 then that means the Nakamoto Bonus is disabled. wdyt? |
IMHO I think it is confusing because we already have some rules, such as if the maximum and minimum are equal, the commission is fixed, and if both are zero, the Nakamoto is disabled. I think it works if it's well documented |
OK as you prefer. |
} | ||
|
||
if !params.NakamotoBonus.Enabled { | ||
// Always set eta to zero and skip dynamic update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Always set eta to zero and skip dynamic update | |
// Always set Nakamoto Bonus to zero and skip dynamic update |
lowAvg := avg(low) | ||
coefficient := nakamotoCoefficient | ||
|
||
// Adjust coefficient: if avgHigh >= 3x avgLow, increase eta, else decrease |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Adjust coefficient: if avgHigh >= 3x avgLow, increase eta, else decrease | |
// Adjust coefficient: if avgHigh >= 3x avgLow, increase Nakamoto Bonus, else decrease |
implementation of https://github.com/atomone-hub/atomone/blob/main/docs/architecture/adr-004-nakamoto-bonus.md