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: add x/farm module #362

Merged
merged 34 commits into from
Oct 5, 2022
Merged

feat: add x/farm module #362

merged 34 commits into from
Oct 5, 2022

Conversation

hallazzang
Copy link
Contributor

@hallazzang hallazzang commented Sep 17, 2022

Description

The new x/farm module will be the successor of the x/farming module.

Tasks

  • Implement Farm, Unfarm, Harvest
  • Implement weight-based reward allocation
  • Write gRPC query endpoints and CLI commands
  • Add public plan proposal and handler
  • Write simulation tests
  • Write unit tests
  • Add invariant checks
  • Optimize gas usage
  • Write documentations
  • Split protobuf messages(Farm, Position, ...)
  • Rename Plan to FarmingPlan?

References


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Appropriate labels applied
  • Targeted PR against correct branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@hallazzang hallazzang self-assigned this Sep 17, 2022
func BeginBlocker(ctx sdk.Context, k keeper.Keeper) {
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)

if err := k.AllocateRewards(ctx); err != nil {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)

if err := k.AllocateRewards(ctx); err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
if err := k.AllocateRewards(ctx); err != nil {
panic(err)
}
k.SetLastBlockTime(ctx, ctx.BlockTime())

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
if err := k.AllocateRewards(ctx); err != nil {
panic(err)
}
k.SetLastBlockTime(ctx, ctx.BlockTime())

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/farm/keeper/plan.go Fixed Show fixed Hide fixed
@jaybxyz jaybxyz mentioned this pull request Sep 20, 2022
26 tasks
func BeginBlocker(ctx sdk.Context, k keeper.Keeper) {
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)

if err := k.TerminateEndedPlans(ctx); err != nil {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)

if err := k.TerminateEndedPlans(ctx); err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
@jaybxyz
Copy link
Contributor

jaybxyz commented Sep 23, 2022

As a side note in case I forget during the future code review, the farm module needs to be defined in labeler.yml file.

x/farm/keeper/farm.go Fixed Show fixed Hide fixed
x/farm/keeper/farm.go Fixed Show fixed Hide fixed
Comment on lines +203 to +205
for _, rewards := range allocs {
totalRewards = totalRewards.Add(rewards...)
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
@hallazzang hallazzang marked this pull request as ready for review September 27, 2022 06:28
Comment on lines +142 to +150
for denom := range totalFarmingAmtByDenom {
if !totalFarmingAmtByDenom[denom].Equal(farmingAmtSumByDenom[denom]) {
msg += fmt.Sprintf(
"\tfarm %s total farming amount %s != sum %s\n",
denom, totalFarmingAmtByDenom[denom], farmingAmtSumByDenom[denom],
)
cnt++
}
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Copy link
Contributor

@jaybxyz jaybxyz left a comment

Choose a reason for hiding this comment

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

Great work on the farm module implementation!

Overall it looks good to me. I have some comments and questions below.

  • The farm module needs to be defined in labeler.yml file.
  • There are some TODO comments that need to be resolved

x/farm/client/cli/tx.go Show resolved Hide resolved
x/farm/types/keys.go Outdated Show resolved Hide resolved
x/farm/types/proposal.go Outdated Show resolved Hide resolved
x/farm/keeper/plan.go Outdated Show resolved Hide resolved
x/farm/keeper/farm.go Outdated Show resolved Hide resolved
x/farm/keeper/farm.go Show resolved Hide resolved
x/farm/keeper/farm.go Outdated Show resolved Hide resolved
x/farm/types/proposal.go Outdated Show resolved Hide resolved
x/farm/abci.go Show resolved Hide resolved
also add explanatory comments for core logics
also add a few safety checks when sending coins
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.

None yet

3 participants