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

feat: observer rewards #855

Merged
merged 57 commits into from
Aug 14, 2023
Merged

feat: observer rewards #855

merged 57 commits into from
Aug 14, 2023

Conversation

kingpinXD
Copy link
Contributor

@kingpinXD kingpinXD commented Jul 26, 2023

Description

  • Add functions to testutil/network for adding crosschain and observer genesis state, which tests can use in all modules. Refactor existing tests to use these functions instead of local counterparts in the module itself

  • Add begin block function in emissions module to calculate and allocate rewards

  • Move Block Reward components to its file under emissons/keeper

  • Add ballot creation height to ballots .

  • Add BallotMaturityHeight param .

  • Add unit tests for BuildRewardsDistribution and DistributeObserverRewards

  • This PR does not add a migration script to modify existing ballots on the testnet , or distribute any of the older rewards

Closes: #839

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@kingpinXD kingpinXD marked this pull request as draft August 1, 2023 14:25
@kingpinXD kingpinXD marked this pull request as ready for review August 8, 2023 22:50
}
rewardPerUnit := sdkmath.ZeroInt()
if totalRewardsUnits > 0 && amount.IsPositive() {
rewardPerUnit = amount.Quo(sdk.NewInt(totalRewardsUnits))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ballots can be cleared after this distribution if needed, decided not to do this in this PR . Can be added in later

Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

👍

Many comments but many of them are small.

proto/emissions/events.proto Outdated Show resolved Hide resolved
proto/emissions/params.proto Outdated Show resolved Hide resolved
proto/emissions/query.proto Outdated Show resolved Hide resolved
testutil/network/genesis_state.go Outdated Show resolved Hide resolved
testutil/network/genesis_state.go Outdated Show resolved Hide resolved
x/observer/genesis.go Show resolved Hide resolved
x/observer/genesis.go Outdated Show resolved Hide resolved
x/observer/keeper/ballot.go Outdated Show resolved Hide resolved
x/observer/keeper/ballot.go Outdated Show resolved Hide resolved
x/observer/types/ballot.go Show resolved Hide resolved
Copy link
Contributor

@kevinssgh kevinssgh left a comment

Choose a reason for hiding this comment

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

Looks good, just a few comments.

proto/emissions/withdrawble_emisions.proto Outdated Show resolved Hide resolved
x/crosschain/keeper/keeper_tss_voter.go Show resolved Hide resolved
x/observer/types/ballot_test.go Show resolved Hide resolved
proto/observer/params.proto Show resolved Hide resolved
Copy link
Collaborator

@brewmaster012 brewmaster012 left a comment

Choose a reason for hiding this comment

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

Overall looks good.
Q: where is the performance of observer votes tracked and rewarded/slashed?

proto/observer/ballot.proto Outdated Show resolved Hide resolved
proto/observer/genesis.proto Outdated Show resolved Hide resolved
proto/observer/params.proto Outdated Show resolved Hide resolved
@kingpinXD
Copy link
Contributor Author

Overall looks good. Q: where is the performance of observer votes tracked and rewarded/slashed?

We are not tracking the performance. Is there a reason to do that? The ballots are not deleted, so we could write an off-chain solution to create that if needed.

Lets discuss this over a GitHub issue if needed

@brewmaster012
Copy link
Collaborator

Ok we can do that separately.

Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

👍

@@ -15,7 +15,8 @@ message GenesisState {
repeated Ballot ballots = 1;
repeated ObserverMapper observers = 2;
repeated NodeAccount nodeAccountList = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

put snake case on this field as well?

@kingpinXD kingpinXD added the v8 label Aug 14, 2023
@kingpinXD kingpinXD merged commit 93fa870 into develop Aug 14, 2023
16 checks passed
@kingpinXD kingpinXD deleted the feature/observer-rewards branch August 14, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

basic framework for reward and punishment for observers & keysigners
5 participants