Issue #111 - Cleanup Utility Module (#119)

# Objective
Reduce the code footprint of the utility module, so it is more understandable to new readers while finding a balance between minimizing legacy complexity in the future and not over-generalizing the existing code.

# Origin Document

PR73 ([First iteration of a PostgreSQL based Persistence Schema](#73)) underwent several iterations in order to make the [persistence module implementation](https://github.com/pokt-network/pocket/tree/main/persistence) have the optionality to remain verbose on a per-actor basis but also avoid code duplication and redundancy.

The first iteration of the [utility module](https://github.com/pokt-network/pocket/tree/main/persistence) did not undergo the same iterations and therefore has some low-hanging fruit to simplify the codebase before further functionality is added.

# Goals / Deliverables
- [x] Extract common actor functionality into general interfaces
- [x] Reduce the code complexity and code footprint so it is more approachable and maintainable
- [ ] Identify and document/cleanup parts of the code that are unclear to new readers
- [x] Generalize both unit tests and functionality where possible
- [x] Maintain verbose entry points for each actor
General:
- [x] Update the CHANGELOG
- [x] Update the README
- [x] Update the global documentation & references
- [x] Document small issues / TODOs along the way

## Testing Methodology
 - **Utility tests**: `make test_utility_*`
 - **All tests**: `make test_all`
 - **LocalNet**: works following the instructions in [docs/development](https://github.com/pokt-network/pocket/tree/main/docs/development)

## Non-goals
- Adding any new functionality

---

## Changes Made
- Removed transaction fees from the transaction structure as fees will be enforced at the state level
- Removed actor specific messages (besides DoubleSign) and added actorType field to the struct
- Removed pause messages and functionality as it is out of scope for the current POS iteration
- Removed session and test-scoring as it's out of scope for the current POS iteration
- Consolidated unit test functionality for actors
- Modified pre-persistence to match persistence for Update(actor), 'amountToAdd' is now just 'amount'

## Future work
- There's more documentation that is needed for this to be complete. In a separate issue, I'd like to tackle connecting Utility with Persistence.
- Further cleanup / refactoring / optimizations can be made, but that can be left to future iteration of the module.

---

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MacBook-Pro-2.local>
Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MBP-2.lan>
Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

Makefile

@@ -27,6 +27,8 @@ help:
 	} \
 	{ lastLine = $$0 }' $(MAKEFILE_LIST)
 
+# Internal helper target - check if docker is installed
+.PHONY: docker_check
 docker_check:
 	{ \
 	if ( ! ( command -v docker >/dev/null && command -v docker-compose >/dev/null )); then \
@@ -35,6 +37,8 @@ docker_check:
 	fi; \
 	}
 
+# Internal helper target - prompt the user before continuing
+.PHONY: prompt_user
 prompt_user:
 	@echo "Are you sure? [y/N] " && read ans && [ $${ans:-N} = y ]
 
@@ -238,12 +242,11 @@ protogen_clean:
 ## Generate go structures for all of the protobufs
 protogen_local:
 	$(eval proto_dir = "./shared/types/proto/")
-
-	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./shared/types/proto             --go_out=./shared/types         ./shared/types/proto/*.proto
-	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./utility/proto                  --go_out=./utility/types        ./utility/proto/*.proto
-	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./shared/types/genesis/proto     --go_out=./shared/types/genesis ./shared/types/genesis/proto/*.proto
-	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./consensus/types/proto          --go_out=./consensus/types      ./consensus/types/proto/*.proto
-	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./p2p/raintree/types/proto --go_out=./p2p/types      ./p2p/raintree/types/proto/*.proto
+	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./shared/types/proto             --go_out=./shared/types         ./shared/types/proto/*.proto         --experimental_allow_proto3_optional
+	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./utility/proto                  --go_out=./utility/types        ./utility/proto/*.proto              --experimental_allow_proto3_optional
+	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./shared/types/genesis/proto     --go_out=./shared/types/genesis ./shared/types/genesis/proto/*.proto --experimental_allow_proto3_optional
+	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./consensus/types/proto          --go_out=./consensus/types      ./consensus/types/proto/*.proto      --experimental_allow_proto3_optional
+	protoc --go_opt=paths=source_relative -I=${proto_dir} -I=./p2p/raintree/types/proto       --go_out=./p2p/types            ./p2p/raintree/types/proto/*.proto   --experimental_allow_proto3_optional
 
 	echo "View generated proto files by running: make protogen_show"
 

README.md

@@ -40,12 +40,12 @@ All the links you'll need are listed below. If you'd like to contribute to the P
 - [Persistence Architecture](persistence/README.md)
 - _(Soon to be deprecated)_ [PrePersistence Architecture](persistence/pre_persistence/README.md)
 - [P2P Architecture](p2p/README.md)
-- [Utility Architecture](utility/README.md)
+- [Utility Architecture](utility/doc/README.md)
 
 ### Changelogs
 
 - [Consensus Changelog](consensus/CHANGELOG.md)
-- [Utility Changelog](utility/CHANGELOG.md)
+- [Utility Changelog](utility/doc/CHANGELOG.md)
 - [Persistence Changelog](persistence/CHANGELOG.md)
 - _Coming Soon: P2P Changelog_
 

persistence/README.md

@@ -157,6 +157,7 @@ Short-term (i.e. simpler starter) tasks:
 - [ ] REFACTOR/DISCUSS: Should we prefix the functions in the `PersistenceModule` with the Param / Actor it's impacting to make autocomplete in implementation better?
 - [ ] DISCUSS: Consider removing all `Set` methods (e.g. `SetAccountAmount`) and replace with `Add` (e.g. `AddAccountAmount`) by having it leverage a "default zero".
 - [ ] REFACTOR(https://github.com/pokt-network/pocket/issues/102): Split `account` and `pool` into a shared actor (e.g. like fisherman/validator/serviceNode/application) and simplify the code in half
+- [ ] CLEANUP: Remove `tokens` or `stakedTokens` in favor of using `amount` everywhere since the denomination is not clear. As a follow up. Consider a massive rename to make the denomination explicit.
 
 Mid-term (i.e. new feature or major refactor) tasks:
 

persistence/application.go

@@ -25,11 +25,11 @@ func (p PostgresContext) GetApp(address []byte, height int64) (operator, publicK
 	return
 }
 
-func (p PostgresContext) InsertApp(address []byte, publicKey []byte, output []byte, _ bool, _ int, maxRelays string, stakedTokens string, chains []string, pausedHeight int64, unstakingHeight int64) error {
+func (p PostgresContext) InsertApp(address []byte, publicKey []byte, output []byte, _ bool, _ int, maxRelays string, stakedAmount string, chains []string, pausedHeight int64, unstakingHeight int64) error {
 	return p.InsertActor(schema.ApplicationActor, schema.BaseActor{
 		Address:            hex.EncodeToString(address),
 		PublicKey:          hex.EncodeToString(publicKey),
-		StakedTokens:       stakedTokens,
+		StakedTokens:       stakedAmount,
 		ActorSpecificParam: maxRelays,
 		OutputAddress:      hex.EncodeToString(output),
 		PausedHeight:       pausedHeight,
@@ -38,10 +38,10 @@ func (p PostgresContext) InsertApp(address []byte, publicKey []byte, output []by
 	})
 }
 
-func (p PostgresContext) UpdateApp(address []byte, maxRelays string, stakedTokens string, chains []string) error {
+func (p PostgresContext) UpdateApp(address []byte, maxRelays string, stakedAmount string, chains []string) error {
 	return p.UpdateActor(schema.ApplicationActor, schema.BaseActor{
 		Address:            hex.EncodeToString(address),
-		StakedTokens:       stakedTokens,
+		StakedTokens:       stakedAmount,
 		ActorSpecificParam: maxRelays,
 		Chains:             chains,
 	})

persistence/fisherman.go

@@ -25,11 +25,11 @@ func (p PostgresContext) GetFisherman(address []byte, height int64) (operator, p
 	return
 }
 
-func (p PostgresContext) InsertFisherman(address []byte, publicKey []byte, output []byte, _ bool, _ int, serviceURL string, stakedTokens string, chains []string, pausedHeight int64, unstakingHeight int64) error {
+func (p PostgresContext) InsertFisherman(address []byte, publicKey []byte, output []byte, _ bool, _ int, serviceURL string, stakedAmount string, chains []string, pausedHeight int64, unstakingHeight int64) error {
 	return p.InsertActor(schema.FishermanActor, schema.BaseActor{
 		Address:            hex.EncodeToString(address),
 		PublicKey:          hex.EncodeToString(publicKey),
-		StakedTokens:       stakedTokens,
+		StakedTokens:       stakedAmount,
 		ActorSpecificParam: serviceURL,
 		OutputAddress:      hex.EncodeToString(output),
 		PausedHeight:       pausedHeight,
@@ -38,10 +38,10 @@ func (p PostgresContext) InsertFisherman(address []byte, publicKey []byte, outpu
 	})
 }
 
-func (p PostgresContext) UpdateFisherman(address []byte, serviceURL string, stakedTokens string, chains []string) error {
+func (p PostgresContext) UpdateFisherman(address []byte, serviceURL string, stakedAmount string, chains []string) error {
 	return p.UpdateActor(schema.FishermanActor, schema.BaseActor{
 		Address:            hex.EncodeToString(address),
-		StakedTokens:       stakedTokens,
+		StakedTokens:       stakedAmount,
 		ActorSpecificParam: serviceURL,
 		Chains:             chains,
 	})

persistence/module.go

@@ -10,6 +10,39 @@ import (
 )
 
 var _ modules.PersistenceModule = &persistenceModule{}
+var _ modules.PersistenceContext = &PostgresContext{}
+
+func (p PostgresContext) GetAppStakeAmount(height int64, address []byte) (string, error) {
+	panic("TODO: implement PostgresContext.GetAppStakeAmount")
+}
+
+func (p PostgresContext) SetAppStakeAmount(address []byte, stakeAmount string) error {
+	panic("TODO: implement PostgresContext.SetAppStakeAmount")
+}
+
+func (p PostgresContext) GetServiceNodeStakeAmount(height int64, address []byte) (string, error) {
+	panic("TODO: implement PostgresContext.GetServiceNodeStakeAmount")
+}
+
+func (p PostgresContext) SetServiceNodeStakeAmount(address []byte, stakeAmount string) error {
+	panic("TODO: implement PostgresContext.SetServiceNodeStakeAmount")
+}
+
+func (p PostgresContext) GetFishermanStakeAmount(height int64, address []byte) (string, error) {
+	panic("TODO: implement PostgresContext.SetServiceNodeStakeAmount")
+}
+
+func (p PostgresContext) SetFishermanStakeAmount(address []byte, stakeAmount string) error {
+	panic("TODO: implement PostgresContext.SetFishermanStakeAmount")
+}
+
+func (p PostgresContext) GetValidatorStakeAmount(height int64, address []byte) (string, error) {
+	panic("TODO: implement PostgresContext.GetValidatorStakeAmount")
+}
+
+func (p PostgresContext) SetValidatorStakeAmount(address []byte, stakeAmount string) error {
+	panic("TODO: implement PostgresContext.SetValidatorStakeAmount")
+}
 
 type persistenceModule struct {
 	bus modules.Bus

persistence/pre_persistence/account.go

@@ -25,7 +25,7 @@ func (m *PrePersistenceContext) SubtractPoolAmount(name string, amount string) e
 	sub := func(s *big.Int, s1 *big.Int) error {
 		s.Sub(s, s1)
 		if s.Sign() == -1 {
-			return types.ErrInsufficientAmountError()
+			return types.ErrInsufficientAmount()
 		}
 		return nil
 	}
@@ -195,7 +195,7 @@ func (m *PrePersistenceContext) SubtractAccountAmount(address []byte, amount str
 	sub := func(s *big.Int, s1 *big.Int) error {
 		s.Sub(s, s1)
 		if s.Sign() == -1 {
-			return types.ErrInsufficientAmountError()
+			return types.ErrInsufficientAmount()
 		}
 		return nil
 	}

persistence/pre_persistence/app.go

@@ -82,7 +82,33 @@ func (m *PrePersistenceContext) GetAllApps(height int64) (apps []*typesGenesis.A
 	return
 }
 
-func (m *PrePersistenceContext) InsertApp(address []byte, publicKey []byte, output []byte, paused bool, status int, maxRelays string, stakedTokens string, chains []string, pausedHeight int64, unstakingHeight int64) error {
+func (m *PrePersistenceContext) GetAppStakeAmount(height int64, address []byte) (string, error) {
+	app, err := m.GetApp(address, height)
+	if err != nil {
+		return "", err
+	}
+	return app.StakedTokens, nil
+}
+
+func (m *PrePersistenceContext) SetAppStakeAmount(address []byte, stakeAmount string) error {
+	codec := types.GetCodec()
+	db := m.Store()
+	app, err := m.GetApp(address, m.Height)
+	if err != nil {
+		return err
+	}
+	if app == nil {
+		return fmt.Errorf("does not exist in world state: %v", address)
+	}
+	app.StakedTokens = stakeAmount
+	bz, err := codec.Marshal(app)
+	if err != nil {
+		return err
+	}
+	return db.Put(append(AppPrefixKey, address...), bz)
+}
+
+func (m *PrePersistenceContext) InsertApp(address []byte, publicKey []byte, output []byte, paused bool, status int, maxRelays string, stakedAmount string, chains []string, pausedHeight int64, unstakingHeight int64) error {
 	height, err := m.GetHeight()
 	if err != nil {
 		return err
@@ -100,7 +126,7 @@ func (m *PrePersistenceContext) InsertApp(address []byte, publicKey []byte, outp
 		Status:          int32(status),
 		Chains:          chains,
 		MaxRelays:       maxRelays,
-		StakedTokens:    stakedTokens,
+		StakedTokens:    stakedAmount,
 		PausedHeight:    pausedHeight,
 		UnstakingHeight: unstakingHeight,
 		Output:          output,
@@ -112,7 +138,7 @@ func (m *PrePersistenceContext) InsertApp(address []byte, publicKey []byte, outp
 	return db.Put(key, bz)
 }
 
-func (m *PrePersistenceContext) UpdateApp(address []byte, maxRelaysToAdd string, amountToAdd string, chainsToUpdate []string) error {
+func (m *PrePersistenceContext) UpdateApp(address []byte, maxRelaysToAdd string, amount string, chainsToUpdate []string) error {
 	height, err := m.GetHeight()
 	if err != nil {
 		return err
@@ -125,15 +151,15 @@ func (m *PrePersistenceContext) UpdateApp(address []byte, maxRelaysToAdd string,
 	db := m.Store()
 	key := append(AppPrefixKey, address...)
 	// compute new values
-	stakedTokens, err := types.StringToBigInt(app.StakedTokens)
-	if err != nil {
-		return err
-	}
-	stakedTokensToAddI, err := types.StringToBigInt(amountToAdd)
-	if err != nil {
-		return err
-	}
-	stakedTokens.Add(stakedTokens, stakedTokensToAddI)
+	//stakedTokens, err := types.StringToBigInt(app.StakedTokens)
+	//if err != nil {
+	//	return err
+	//}
+	stakedTokens, err := types.StringToBigInt(amount)
+	//if err != nil {
+	//	return err
+	//}
+	//stakedTokens.Add(stakedTokens, stakedTokensToAddI)
 	maxRelays, err := types.StringToBigInt(app.MaxRelays)
 	if err != nil {
 		return err
@@ -310,7 +336,7 @@ func (m *PrePersistenceContext) SetAppPauseHeight(address []byte, height int64)
 	if app == nil {
 		return fmt.Errorf("does not exist in world state: %v", address)
 	}
-	if app.PausedHeight == types.HeightNotUsed {
+	if height != types.HeightNotUsed {
 		app.Paused = true
 	} else {
 		app.Paused = false

persistence/pre_persistence/app_test.go

@@ -115,19 +115,14 @@ func TestUpdateApp(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	before, err := ctx.(*PrePersistenceContext).GetApp(actor.Address, height)
-	require.NoError(t, err)
-	tokens := before.StakedTokens
-	bigBeforeTokens, err := types.StringToBigInt(tokens)
 	require.NoError(t, err)
 	err = ctx.UpdateApp(actor.Address, zero, one, typesGenesis.DefaultChains)
 	require.NoError(t, err)
 	got, err := ctx.(*PrePersistenceContext).GetApp(actor.Address, height)
 	require.NoError(t, err)
 	bigAfterTokens, err := types.StringToBigInt(got.StakedTokens)
 	require.NoError(t, err)
-	bigAfterTokens.Sub(bigAfterTokens, bigBeforeTokens)
-	if bigAfterTokens.Cmp(bigExpectedTokens) != 0 {
+	if bigExpectedTokens.Cmp(bigAfterTokens) != 0 {
 		t.Fatal("incorrect after balance")
 	}
 }

persistence/pre_persistence/fisherman.go

@@ -11,6 +11,10 @@ import (
 	"google.golang.org/protobuf/proto"
 )
 
+func (m *PrePersistenceContext) GetLatestBlockHeight() (uint64, error) {
+	return uint64(m.Height), nil
+}
+
 func (m *PrePersistenceContext) GetFishermanExists(address []byte, height int64) (exists bool, err error) {
 	db := m.Store()
 	key := append(FishermanPrefixKey, address...)
@@ -82,7 +86,33 @@ func (m *PrePersistenceContext) GetAllFishermen(height int64) (fishermen []*type
 	return
 }
 
-func (m *PrePersistenceContext) InsertFisherman(address []byte, publicKey []byte, output []byte, paused bool, status int, serviceURL string, stakedTokens string, chains []string, pausedHeight int64, unstakingHeight int64) error {
+func (m *PrePersistenceContext) GetFishermanStakeAmount(height int64, address []byte) (string, error) {
+	fish, _, err := m.GetFisherman(address, height)
+	if err != nil {
+		return "", err
+	}
+	return fish.StakedTokens, nil
+}
+
+func (m *PrePersistenceContext) SetFishermanStakeAmount(address []byte, stakeAmount string) error {
+	codec := types.GetCodec()
+	db := m.Store()
+	fish, _, err := m.GetFisherman(address, m.Height)
+	if err != nil {
+		return err
+	}
+	if fish == nil {
+		return fmt.Errorf("does not exist in world state: %v", address)
+	}
+	fish.StakedTokens = stakeAmount
+	bz, err := codec.Marshal(fish)
+	if err != nil {
+		return err
+	}
+	return db.Put(append(FishermanPrefixKey, address...), bz)
+}
+
+func (m *PrePersistenceContext) InsertFisherman(address []byte, publicKey []byte, output []byte, paused bool, status int, serviceURL string, stakedAmount string, chains []string, pausedHeight int64, unstakingHeight int64) error {
 	height, err := m.GetHeight()
 	if err != nil {
 		return err
@@ -100,7 +130,7 @@ func (m *PrePersistenceContext) InsertFisherman(address []byte, publicKey []byte
 		Status:          int32(status),
 		Chains:          chains,
 		ServiceUrl:      serviceURL,
-		StakedTokens:    stakedTokens,
+		StakedTokens:    stakedAmount,
 		PausedHeight:    pausedHeight,
 		UnstakingHeight: unstakingHeight,
 		Output:          output,
@@ -112,7 +142,7 @@ func (m *PrePersistenceContext) InsertFisherman(address []byte, publicKey []byte
 	return db.Put(key, bz)
 }
 
-func (m *PrePersistenceContext) UpdateFisherman(address []byte, serviceURL string, amountToAdd string, chains []string) error {
+func (m *PrePersistenceContext) UpdateFisherman(address []byte, serviceURL string, amount string, chains []string) error {
 	height, err := m.GetHeight()
 	if err != nil {
 		return err
@@ -125,15 +155,15 @@ func (m *PrePersistenceContext) UpdateFisherman(address []byte, serviceURL strin
 	db := m.Store()
 	key := append(FishermanPrefixKey, address...)
 	// compute new values
-	stakedTokens, err := types.StringToBigInt(fish.StakedTokens)
+	//stakedTokens, err := types.StringToBigInt(fish.StakedTokens)
+	//if err != nil {
+	//	return err
+	//}
+	stakedTokens, err := types.StringToBigInt(amount)
 	if err != nil {
 		return err
 	}
-	stakedTokensToAddI, err := types.StringToBigInt(amountToAdd)
-	if err != nil {
-		return err
-	}
-	stakedTokens.Add(stakedTokens, stakedTokensToAddI)
+	//stakedTokens.Add(stakedTokens, stakedTokensToAddI)
 	// update values
 	fish.ServiceUrl = serviceURL
 	fish.StakedTokens = types.BigIntToString(stakedTokens)
@@ -294,10 +324,10 @@ func (m *PrePersistenceContext) SetFishermanPauseHeight(address []byte, height i
 	if !exists {
 		return fmt.Errorf("does not exist in world state")
 	}
-	if height == types.HeightNotUsed {
-		fish.Paused = false
-	} else {
+	if height != types.HeightNotUsed {
 		fish.Paused = true
+	} else {
+		fish.Paused = false
 	}
 	fish.PausedHeight = height
 	bz, err := codec.Marshal(fish)

persistence/pre_persistence/fisherman_test.go

@@ -114,19 +114,14 @@ func TestUpdateFisherman(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	before, _, err := ctx.(*PrePersistenceContext).GetFisherman(actor.Address, height)
-	require.NoError(t, err)
-	tokens := before.StakedTokens
-	bigBeforeTokens, err := types.StringToBigInt(tokens)
 	require.NoError(t, err)
 	err = ctx.UpdateFisherman(actor.Address, zero, one, typesGenesis.DefaultChains)
 	require.NoError(t, err)
 	got, _, err := ctx.(*PrePersistenceContext).GetFisherman(actor.Address, height)
 	require.NoError(t, err)
 	bigAfterTokens, err := types.StringToBigInt(got.StakedTokens)
 	require.NoError(t, err)
-	bigAfterTokens.Sub(bigAfterTokens, bigBeforeTokens)
-	if bigAfterTokens.Cmp(bigExpectedTokens) != 0 {
+	if bigExpectedTokens.Cmp(bigAfterTokens) != 0 {
 		t.Fatal("incorrect after balance")
 	}
 }