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.

Sign up for GitHub

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

Jump to bottom

revise self stake buckets #4312

Merged
merged 2 commits into from
Jun 27, 2024
Merged

revise self stake buckets #4312

merged 2 commits into from
Jun 27, 2024

Conversation

CoderZhi
Copy link
Collaborator

@CoderZhi CoderZhi commented Jun 25, 2024 •
edited
Loading

jump to https://github.com/iotexproject/iotex-core/pull/4312/files/c9cbbe0ca978ead3ff6a8c9d87cb8ff4c935c5f3..7536390e8fbcde722b1209ef5676d3b2b2adabdc to review. add a feature to revise endorsements of self stake buckets

Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • [] Code refactor or improvement
  • [] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
  • [] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • [] make test
  • [] fullsync
  • [] Other test (please specify)

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • [] My code follows the style guidelines of this project
  • [] I have performed a self-review of my code
  • [] I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • [] My changes generate no new warnings
  • [] I have added tests that prove my fix is effective or that my feature works
  • [] New and existing unit tests pass locally with my changes
  • [] Any dependent changes have been merged and published in downstream modules

@CoderZhi CoderZhi requested review from dustinxie, Liuhaai, envestcc and a team as code owners June 25, 2024 00:59
@envestcc envestcc force-pushed the revise_self_stake_buckets branch from 4c07942 to cfe1123 Compare June 25, 2024 14:48
CoderZhi
CoderZhi commented Jun 25, 2024
View reviewed changes
action/candidate_endorsement.go Outdated Show resolved Hide resolved
action/candidate_endorsement.go Outdated Show resolved Hide resolved
action/candidate_endorsement.go Outdated Show resolved Hide resolved
action/protocol/staking/handler_candidate_endorsement.go Outdated Show resolved Hide resolved
@envestcc envestcc force-pushed the revise_self_stake_buckets branch 2 times, most recently from 37f7c45 to 20bee9e Compare June 26, 2024 06:08
@CoderZhi CoderZhi force-pushed the revise_self_stake_buckets branch from a5a4d79 to 7536390 Compare June 26, 2024 15:55
dustinxie
dustinxie reviewed Jun 26, 2024
View reviewed changes
action/protocol/staking/protocol.go Outdated
// new vote reviser, revise ate greenland
voteReviser := NewVoteReviser(cfg.Staking.VoteWeightCalConsts, correctCandsHeight, reviseHeights...)
// new vote reviser, revise at greenland
// TODO: pass ctx to voteReviser
Copy link
Member

Choose a reason for hiding this comment

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

think this is done below? or still needed?

dustinxie
dustinxie reviewed Jun 26, 2024
View reviewed changes
action/protocol/staking/protocol_test.go Outdated
}, nil, nil, nil, genesis.Default.GreenlandBlockHeight)
Revise: ReviseConfig{
VoteWeight: genesis.Default.Staking.VoteWeightCalConsts,
CorrectCandsHeight: genesis.Default.GreenlandBlockHeight,
Copy link
Member

Choose a reason for hiding this comment

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

correct or leave empty

dustinxie
dustinxie reviewed Jun 26, 2024
View reviewed changes
action/protocol/staking/protocol_test.go Outdated
}, nil, nil, nil, genesis.Default.GreenlandBlockHeight, genesis.Default.GreenlandBlockHeight)
Revise: ReviseConfig{
VoteWeight: genesis.Default.Staking.VoteWeightCalConsts,
CorrectCandsHeight: genesis.Default.GreenlandBlockHeight,
Copy link
Member

Choose a reason for hiding this comment

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

correct or leave empty

dustinxie
dustinxie reviewed Jun 26, 2024
View reviewed changes
action/protocol/staking/protocol_test.go Outdated
}, cbi, nil, nil, genesis.Default.GreenlandBlockHeight, genesis.Default.GreenlandBlockHeight)
Revise: ReviseConfig{
VoteWeight: genesis.Default.Staking.VoteWeightCalConsts,
CorrectCandsHeight: genesis.Default.GreenlandBlockHeight,
Copy link
Member

Choose a reason for hiding this comment

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

correct or leave empty

dustinxie
dustinxie reviewed Jun 26, 2024
View reviewed changes
action/protocol/staking/protocol_test.go Outdated
}, nil, nil, nil, genesis.Default.GreenlandBlockHeight)
Revise: ReviseConfig{
VoteWeight: genesis.Default.Staking.VoteWeightCalConsts,
CorrectCandsHeight: genesis.Default.GreenlandBlockHeight,
Copy link
Member

Choose a reason for hiding this comment

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

correct or leave empty

dustinxie
dustinxie reviewed Jun 26, 2024
View reviewed changes
action/protocol/staking/protocol_test.go Outdated
}, nil, csIndexer, nil, genesis.Default.GreenlandBlockHeight)
Revise: ReviseConfig{
VoteWeight: genesis.Default.Staking.VoteWeightCalConsts,
CorrectCandsHeight: genesis.Default.GreenlandBlockHeight,
Copy link
Member

Choose a reason for hiding this comment

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

correct or leave empty

dustinxie
dustinxie reviewed Jun 26, 2024
View reviewed changes
action/protocol/staking/vote_reviser.go Outdated
case err != nil:
return err
}
cands, err = vr.reviseSelfStakeBuckets(ctx, csm, height, cands)
Copy link
Member

Choose a reason for hiding this comment

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

revise self stake is an option for this time, not needed for every revise, so better to write like

if vr.shouldReviseSelfStakeBuckets(height) {
    cands, err = vr.reviseSelfStakeBuckets()
}

Copy link
Member

@dustinxie dustinxie Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

also it's possible to only revise affected cands right? like

if vr.shouldReviseSelfStakeBuckets(height) {
    cands, err := vr.reviseSelfStakeBuckets()
}

cands returned above should only contain affected cands (endorsement expired), we only need to revise the votes for these cands right?

dustinxie
dustinxie reviewed Jun 26, 2024
View reviewed changes
action/rlp_tx_test.go Outdated Show resolved Hide resolved
dustinxie
dustinxie reviewed Jun 26, 2024
View reviewed changes
action/rlp_tx_test.go Outdated Show resolved Hide resolved
@envestcc envestcc force-pushed the revise_self_stake_buckets branch from 7536390 to 85d7136 Compare June 27, 2024 03:29
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
26.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

dustinxie
dustinxie reviewed Jun 27, 2024
View reviewed changes
action/protocol/staking/vote_reviser.go
cands, err = vr.reviseSelfStakeBuckets(ctx, csm, height, cands)
if err != nil {
return err
if vr.shouldReviseSelfStakeBuckets(height) {
Copy link
Member

Choose a reason for hiding this comment

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

it is actually revise for endorsement status, i think name it reviseEndorsement or at least put endorsement into the func name

@envestcc envestcc merged commit 881fc5d into master Jun 27, 2024
2 of 3 checks passed
@envestcc envestcc deleted the revise_self_stake_buckets branch June 27, 2024 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@envestcc envestcc envestcc approved these changes

@dustinxie dustinxie dustinxie approved these changes

@Liuhaai Liuhaai Awaiting requested review from Liuhaai Liuhaai is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CoderZhi @dustinxie @envestcc