Review #190

migueldingli1997 · 2021-10-24T18:23:17Z

Description

Code review of the entire module. Refer to the Files changed for most of the review comments.

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~~

This reverts commit e8640fa.

migueldingli1997

Left some extra comments

migueldingli1997 · 2021-10-25T16:23:13Z

x/farming/keeper/genesis.go

 	k.SetCurrentEpochDays(ctx, genState.CurrentEpochDays)
-	moduleAcc := k.accountKeeper.GetModuleAccount(ctx, types.ModuleName)
-	k.accountKeeper.SetModuleAccount(ctx, moduleAcc)
+	k.accountKeeper.GetModuleAccount(ctx, types.ModuleName)


The GetModuleAccount function creates the module account if it does not exist. This is similar to what the mint and auth modules do. No need to get and set; get is enough.

It will be applied on #202, Thanks 🙇

migueldingli1997 · 2021-10-25T16:25:29Z

x/farming/keeper/staking.go

-			removedFromStaking := queuedStaking.Amount.Neg()
-			queuedStaking.Amount = sdk.ZeroInt()
+			removedFromStaking := queuedStaking.Amount.Neg() // make negative a positive
+			staking.Amount = staking.Amount.Sub(removedFromStaking)


Changed to .Sub for readability

It will be applied on #202, Thank you.

migueldingli1997 · 2021-10-25T16:26:19Z

x/farming/keeper/staking.go

+			k.DeleteQueuedStaking(ctx, coin.Denom, farmerAcc)
 			k.DecreaseTotalStakings(ctx, coin.Denom, removedFromStaking)
-		}
-
-		if queuedStaking.Amount.IsPositive() {
+		} else if queuedStaking.Amount.IsPositive() {
 			k.SetQueuedStaking(ctx, coin.Denom, farmerAcc, queuedStaking)
 		} else {
 			k.DeleteQueuedStaking(ctx, coin.Denom, farmerAcc)


Joined conditions for readability. This required adding a call to .DeleteQueuedStaking in the if clause

Fixed in 2b08a97 (not merged)

migueldingli1997 · 2021-10-25T16:31:52Z

Apart from the comments and suggestions in the Files changed section; in general, more documentation (both inline and in the spec) are needed. This is not a simple module, and so the documentation requirements are greater.

dongsam · 2021-10-26T02:59:28Z

@migueldingli1997 Thank you for your valuable review and suggestion. Would it be better to commit the modifications you suggested on this PR? Or the suggestions and fix commit may be mixed, so should we just talk here and proceed with the fix in a separate PR?

migueldingli1997 · 2021-10-26T08:07:55Z

@dongsam Let's just talk here and proceed with any fixes that we agree upon in a separate PR :)

jaybxyz

Appreciate for reviewing the farming module @migueldingli1997 ! They are so valuable feedbacks and suggestions.

README.md

app/app.go

proto/tendermint/farming/v1beta1/proposal.proto

proto/tendermint/farming/v1beta1/query.proto

x/farming/keeper/proposal_handler.go

x/farming/keeper/staking.go

hallazzang · 2021-10-26T14:20:08Z

x/farming/abci.go

@@ -17,6 +17,7 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) {
 		if !plan.GetTerminated() && ctx.BlockTime().After(plan.GetEndTime()) {
 			if err := k.TerminatePlan(ctx, plan); err != nil {
 				panic(err)
+				// Consider deleting terminated plans


We've discussed about this internally before, and we decided not to delete terminated plans, to provide historical information.

hallazzang · 2021-10-26T14:21:50Z

x/farming/keeper/genesis.go

@@ -19,9 +19,9 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState) {

 	k.SetParams(ctx, genState.Params)
 	// TODO: what if CurrentEpochDays field was empty?
+	// ^ If it is empty, it will default to zero and the following error comes up: 'current epoch days must be positive'


It'll be fixed through #192.

hallazzang · 2021-10-26T14:22:11Z

x/farming/keeper/reward.go

@@ -210,6 +210,7 @@ func (k Keeper) WithdrawRewards(ctx sdk.Context, farmerAcc sdk.AccAddress, staki

 	currentEpoch := k.GetCurrentEpoch(ctx, stakingCoinDenom)
 	// TODO: handle if currentEpoch is 0
+	// ^ Ensure this is handled


It'll be fixed through #192.

hallazzang · 2021-10-26T15:43:33Z

x/farming/keeper/reward.go

+			// Consider adding totalRewards to list of events, so that this shows up when querying the transaction.
+			// However I understand that WithdrawRewards itself does this for each denom one by one.


Sounds good to add total rewards withdrawn in both messages.

Let me fix this.

Added event attribute in de9ca0c (not merged)

hallazzang · 2021-10-26T15:43:48Z

x/farming/keeper/reward.go

@@ -303,6 +306,8 @@ func (k Keeper) AllocationInfos(ctx sdk.Context) []AllocationInfo {
 	farmingPoolBalances := make(map[string]sdk.Coins)   // farmingPoolAddress => sdk.Coins
 	allocCoins := make(map[string]map[uint64]sdk.Coins) // farmingPoolAddress => (planId => sdk.Coins)

+	// This complex function is lacking inline documentation


Let me add inline documentation around the code.

Added comments in b83ea9a (not merged)

hallazzang · 2021-10-26T15:44:20Z

x/farming/keeper/reward.go

@@ -363,6 +369,8 @@ func (k Keeper) AllocationInfos(ctx sdk.Context) []AllocationInfo {
 func (k Keeper) AllocateRewards(ctx sdk.Context) error {
 	unitRewardsByDenom := map[string]sdk.DecCoins{} // (staking coin denom) => (unit rewards)

+	// This complex function is lacking inline documentation


Let me add internal documentation around the code.

Added comments in b83ea9a (not merged)

hallazzang · 2021-10-26T15:55:29Z

x/farming/keeper/reward.go

@@ -379,8 +387,7 @@ func (k Keeper) AllocateRewards(ctx sdk.Context) error {
 				continue
 			}

-			weightProportion := weight.Amount.QuoTruncate(totalWeight)
-			allocCoins, _ := sdk.NewDecCoinsFromCoins(allocInfo.Amount...).MulDecTruncate(weightProportion).TruncateDecimal()
+			allocCoins, _ := sdk.NewDecCoinsFromCoins(allocInfo.Amount...).MulDecTruncate(weight.Amount).TruncateDecimal()


If we don't need to calculate totalWeight anymore(since it'll always be 1 because of the validation logic in msg/proposal handlers) , we can accept this change.

Let me fix this.

Yep that's the context, since totalWeight is expected to always be 1 👍

Fixed in 4f0ea4a (not merged)

x/farming/keeper/staking.go

hallazzang · 2021-10-26T16:01:32Z

x/farming/keeper/plan.go

+		// ^ No need to check if balance has enough to pay for fee. Can just try to .SendCoins; it will error if insufficient tokens
+		// If we want a specific error, the error returned by SendCoins can be replaced by the above error.


That's true. I'll fix this.

Fixed in cdd32e3 (not merged now)

app/app.go

Co-authored-by: Dylan Frendo <ferendo@users.noreply.github.com>

app/app.go

hallazzang · 2021-10-28T07:08:21Z

x/farming/keeper/plan.go

+		// ^ No need to check if balance has enough to pay for fee. Can just try to .SendCoins; it will error if insufficient tokens
+		// If we want a specific error, the error returned by SendCoins can be replaced by the above error.


Fixed in cdd32e3 (not merged now)

hallazzang · 2021-10-28T07:09:35Z

x/farming/keeper/plan.go

+			return nil, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, "lack of %s coins to pay private plan creation fee", diffs.String())
 		}
+		// ^ No need to check if balance has enough to pay for fee. Can just try to .SendCoins; it will error if insufficient tokens
+		// If we want a specific error, the error returned by SendCoins can be replaced by the above error.


Fixed in cdd32e3 (not merged now)

hallazzang · 2021-10-28T07:10:20Z

x/farming/keeper/reward.go

+			// Consider adding totalRewards to list of events, so that this shows up when querying the transaction.
+			// However I understand that WithdrawRewards itself does this for each denom one by one.


Added event attribute in de9ca0c (not merged)

hallazzang · 2021-10-28T07:11:51Z

x/farming/keeper/reward.go

@@ -303,6 +306,8 @@ func (k Keeper) AllocationInfos(ctx sdk.Context) []AllocationInfo {
 	farmingPoolBalances := make(map[string]sdk.Coins)   // farmingPoolAddress => sdk.Coins
 	allocCoins := make(map[string]map[uint64]sdk.Coins) // farmingPoolAddress => (planId => sdk.Coins)

+	// This complex function is lacking inline documentation


Added comments in b83ea9a (not merged)

hallazzang · 2021-10-28T07:12:14Z

x/farming/keeper/reward.go

@@ -336,9 +341,10 @@ func (k Keeper) AllocationInfos(ctx sdk.Context) []AllocationInfo {
 	}

 	var allocInfos []AllocationInfo
-	for farmingPool, coins := range allocCoins {
+	// Renamed coins -> planCoins to make it more clear that it's a map (planId => sdk.Coins) not just sdk.Coins


Fixed in b83ea9a (not merged)

hallazzang · 2021-10-28T07:13:08Z

x/farming/keeper/reward.go

@@ -363,6 +369,8 @@ func (k Keeper) AllocationInfos(ctx sdk.Context) []AllocationInfo {
 func (k Keeper) AllocateRewards(ctx sdk.Context) error {
 	unitRewardsByDenom := map[string]sdk.DecCoins{} // (staking coin denom) => (unit rewards)

+	// This complex function is lacking inline documentation


Added comments in b83ea9a (not merged)

hallazzang · 2021-10-28T07:22:12Z

x/farming/keeper/reward.go

@@ -379,8 +387,7 @@ func (k Keeper) AllocateRewards(ctx sdk.Context) error {
 				continue
 			}

-			weightProportion := weight.Amount.QuoTruncate(totalWeight)
-			allocCoins, _ := sdk.NewDecCoinsFromCoins(allocInfo.Amount...).MulDecTruncate(weightProportion).TruncateDecimal()
+			allocCoins, _ := sdk.NewDecCoinsFromCoins(allocInfo.Amount...).MulDecTruncate(weight.Amount).TruncateDecimal()


Fixed in 4f0ea4a (not merged)

hallazzang · 2021-10-28T07:35:57Z

x/farming/keeper/staking.go

+			k.DeleteQueuedStaking(ctx, coin.Denom, farmerAcc)
 			k.DecreaseTotalStakings(ctx, coin.Denom, removedFromStaking)
-		}
-
-		if queuedStaking.Amount.IsPositive() {
+		} else if queuedStaking.Amount.IsPositive() {
 			k.SetQueuedStaking(ctx, coin.Denom, farmerAcc, queuedStaking)
 		} else {
 			k.DeleteQueuedStaking(ctx, coin.Denom, farmerAcc)


Fixed in 2b08a97 (not merged)

dongsam · 2021-11-24T04:56:21Z

x/farming/spec/01_concepts.md

@@ -14,7 +14,7 @@ There are two types of farming plans in the `farming` module as below.
 A public farming plan can only be created through governance proposal meaning that the proposal must be first agreed and passed in order to create a public plan.
 ### 2. Private Farming Plan

-A private farming plan can be created with any account. The plan creator's account is used as `TerminationAddress`. There is a fee `PlanCreationFee` paid upon plan creation to prevent from spamming attack. 
+A private farming plan can be created with any account. The plan creator's account is used as `TerminationAddress` (consider explaining what a TerminationAddress is used for here). There is a fee `PlanCreationFee` paid upon plan creation to prevent from spamming attack. 


It will be added on #232, Thanks!

dongsam · 2021-11-24T04:57:01Z

x/farming/client/cli/tx.go

+				if clientCtx.Offline {
+					return fmt.Errorf("cannot generate tx in offline mode with --all flag")
+				}
+


I cherry-picked this commit on #232, Thanks for the suggestion.

dongsam · 2021-11-24T05:53:40Z

Close the issue because all suggestions have been discussed or resolved. Thank you again :)

migueldingli1997 added 10 commits October 24, 2021 19:39

Changed get and set of module account to just get

8e168a9

Add suggestion about maccPerms

ad4e626

Add suggestion about NewPublicPlanProposal

42e9e16

Add comment about missing doc

dac0a70

Add comment about doc

e94ce60

Fix typo

89bcd3b

Add suggestion around charging of PrivatePlanCreationFee

48a9a21

Add suggestion around PublicPlanProposal naming

8f99873

Added suggestion about offline generation of MsgHarvest

1df697f

Add suggestion about deleting terminated plans

3aeb70d

migueldingli1997 marked this pull request as draft October 24, 2021 18:24

migueldingli1997 added 13 commits October 24, 2021 20:33

Added note to TODO

70ed1f1

Add another comment for charging of plan creation fee

d9d192c

Add suggestion about AddrAcc

407453d

Simplify coin to string

e8640fa

Change if to else if

206fb96

Simplify Unstake steps

d3f90ed

Revert "Simplify coin to string"

0c9c7a8

This reverts commit e8640fa.

Add question to TODO

dbddcd7

Add event suggestion

f9127f3

Grammar fixes

8f3195e

Add suggestion about coins -> planCoins

80480d7

Add note around TODO

ceb902a

Remove extra comment

d2ae935

migueldingli1997 commented Oct 25, 2021

View reviewed changes

migueldingli1997 marked this pull request as ready for review October 25, 2021 16:31

migueldingli1997 changed the title ~~Draft: Review~~ Review Oct 26, 2021

jaybxyz reviewed Oct 26, 2021

View reviewed changes

README.md Show resolved Hide resolved

app/app.go Show resolved Hide resolved

proto/tendermint/farming/v1beta1/proposal.proto Show resolved Hide resolved

proto/tendermint/farming/v1beta1/query.proto Show resolved Hide resolved

x/farming/keeper/proposal_handler.go Show resolved Hide resolved

dferendo reviewed Oct 26, 2021

View reviewed changes

x/farming/keeper/staking.go Outdated Show resolved Hide resolved

jaybxyz mentioned this pull request Oct 26, 2021

feat: change naming for consistency in public proposals #197

Closed

4 tasks

hallazzang reviewed Oct 26, 2021

View reviewed changes

dongsam mentioned this pull request Oct 27, 2021

Audit: suggestions of SimplyVC #199

Closed

jaybxyz mentioned this pull request Oct 27, 2021

feat: change naming for consistency in public proposals #198

Merged

14 tasks

dongsam reviewed Oct 27, 2021

View reviewed changes

app/app.go Outdated Show resolved Hide resolved

Update x/farming/keeper/staking.go

19d6e80

Co-authored-by: Dylan Frendo <ferendo@users.noreply.github.com>

migueldingli1997 commented Oct 27, 2021

View reviewed changes

app/app.go Outdated Show resolved Hide resolved

Update app/app.go

7722df4

hallazzang mentioned this pull request Oct 27, 2021

fix: apply suggestions from the audit review #202

Merged

14 tasks

hallazzang reviewed Oct 28, 2021

View reviewed changes

dongsam mentioned this pull request Nov 24, 2021

fix: add error for all flag with offline, update docs for TerminationAddress #232

Merged

11 tasks

dongsam reviewed Nov 24, 2021

View reviewed changes

dongsam closed this Nov 24, 2021

		// Consider adding totalRewards to list of events, so that this shows up when querying the transaction.
		// However I understand that WithdrawRewards itself does this for each denom one by one.

		// ^ No need to check if balance has enough to pay for fee. Can just try to .SendCoins; it will error if insufficient tokens
		// If we want a specific error, the error returned by SendCoins can be replaced by the above error.

Review #190

Review #190

Conversation

migueldingli1997 commented Oct 24, 2021 • edited Loading

Description

migueldingli1997 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

migueldingli1997 commented Oct 25, 2021

dongsam commented Oct 26, 2021

migueldingli1997 commented Oct 26, 2021

jaybxyz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongsam commented Nov 24, 2021

migueldingli1997 commented Oct 24, 2021 •

edited

Loading

jaybxyz left a comment •

edited

Loading