[Review] Followups after #552 review (#706)

Minor follows-ups (improvements & capturing TODOs) while reviewing #552 after it was merged.

Nothing is critical, but capturing work and improvements while it's fresh.

Makefile

@@ -307,7 +307,11 @@ docker_wipe: check_docker warn_destructive prompt_user ## [WARNING] Remove all t
 ########################
 
 .PHONY: localnet_up
-localnet_up: check_docker_ps check_kind_context proto_regen localnet_regenesis ## Starts localnet
+localnet_up: check_docker_ps check_kind_context proto_regen localnet_regenesis ## Starts up a clean localnet
+	tilt up
+
+.PHONY: localnet_up_quick
+localnet_up_quick: check_docker_ps check_kind_context ## Starts up a localnet without regenerating fixtures
 	tilt up
 
 .PHONY: localnet_down
@@ -419,9 +423,12 @@ test_load_relays_stress_localnet: test_e2e_env warn_message_local_stress_test ##
 test_verbose: check_go_version ## Run all go tests verbosely
 	go test -count=1 -v -race -tags test ./...
 
+# NB: buildmode=pie is necessary to avoid linker errors on macOS.
+# It is not compatible with `-race`, which is why it's omitted here.
+# See ref for more details: https://github.com/golang/go/issues/54482#issuecomment-1251124908
 .PHONY: test_all
 test_all: warn_flaky_tests check_go_version ## Run all go tests showing detailed output only on failures
-	go test -count=1 -race -tags test ./...
+	go test -count=1 -buildmode=pie -tags test ./...
 
 .PHONY: test_all_with_integration
 test_all_with_integration: check_go_version ## Run all go tests, including those with the integration

e2e/tests/service.feature

@@ -0,0 +1,10 @@
+Feature: Service Namespace
+
+    # TODO_TEST: Implement the scenarios listed below for full e2e coverage.
+
+    # Scenario: A source owner successfully adds a new service that did not exist before
+    # Scenario: A source owner successfully updates a service they created
+    # Scenario: A source owner successfully receives rewards for a service they created
+    # Scenario: A source owner fails to update a service they did not create
+    # Scenario: A source owner updates compute units per relay
+    # Sceneario(post mainnet): A source owner changes the source owner to another address

pkg/relayer/miner/miner_test.go

@@ -62,7 +62,7 @@ func TestMiner_MinedRelays(t *testing.T) {
 
 	// Publish unminable relay fixtures to the mock relays observable.
 	publishRelayFixtures(t, marshaledUnminableRelaysHex, relaysFixturePublishCh)
-	time.Sleep(time.Millisecond)
+	time.Sleep(100 * time.Millisecond)
 
 	// Assert that no unminable relay fixtures were published to minedRelays.
 	actualMinedRelaysMu.Lock()
@@ -71,11 +71,15 @@ func TestMiner_MinedRelays(t *testing.T) {
 
 	// Publish minable relay fixtures to the relay fixtures observable.
 	publishRelayFixtures(t, marshaledMinableRelaysHex, relaysFixturePublishCh)
-	time.Sleep(time.Millisecond)
+	time.Sleep(100 * time.Millisecond)
 
 	// Assert that all minable relay fixtures were published to minedRelays.
 	actualMinedRelaysMu.Lock()
-	require.EqualValues(t, expectedMinedRelays, actualMinedRelays, "TODO_FLAKY: Try re-running with 'go test -v -count=1 -run TestMiner_MinedRelays ./pkg/relayer/miner/...'")
+	// NB: We are comparing the lengths of the expected and actual relays instead of
+	// the actual structures to simplify debugging. When there is an error, the output
+	// is incomprehensible. The developer is expected to debug if this fails due to
+	// a non-flaky reason.
+	require.Equal(t, len(expectedMinedRelays), len(actualMinedRelays), "TODO_FLAKY: Try re-running with 'go test -v -count=1 -run TestMiner_MinedRelays ./pkg/relayer/miner/...'")
 	actualMinedRelaysMu.Unlock()
 }
 

proto/poktroll/shared/service.proto

@@ -16,7 +16,9 @@ message Service {
   // TODO_BETA: Name is currently unused but acts as a reminder that an optional onchain representation of the service is necessary
   string name = 2; // (Optional) Semantic human readable name for the service
 
-  // Used alongside the global 'compute_units_to_tokens_multipler' to calculate the cost of a relay for this service
+  // The cost of a single relay for this service in terms of compute units.
+  // Must be used alongside the global 'compute_units_to_tokens_multipler' to calculate the cost of a relay for this service.
+  // cost_per_relay_for_specific_service = compute_units_per_relay_for_specific_service * compute_units_to_tokens_multipler_global_value
   uint64 compute_units_per_relay = 3; // Compute units required per relay for this service
 
   // The owner address that created the service.

testutil/keeper/tokenomics.go

@@ -69,18 +69,23 @@ type TokenomicsModuleKeepers struct {
 
 // TokenomicsKeepersOpt is a function which receives and potentially modifies the context
 // and tokenomics keepers during construction of the aggregation.
-type TokenomicsKeepersOpt func(context.Context, *TokenomicsModuleKeepers) context.Context
+type TokenomicsModuleKeepersOpt func(context.Context, *TokenomicsModuleKeepers) context.Context
 
 func TokenomicsKeeper(t testing.TB) (tokenomicsKeeper tokenomicskeeper.Keeper, ctx context.Context) {
 	t.Helper()
 	k, ctx, _, _ := TokenomicsKeeperWithActorAddrs(t, nil)
 	return k, ctx
 }
 
-// TODO_TECHDEBT: Have the callers use the keepers to find `appAddr` and `supplierAddr`
-// rather than returning them explicitly.
-// TODO_TECHDEBT(@Olshansk): Remove `service` parameter and convert proper options.
-func TokenomicsKeeperWithActorAddrs(t testing.TB, service *sharedtypes.Service) (
+// TODO_TECHDEBT: Remove this and force everyone to use NewTokenomicsModuleKeepers.
+// There is a difference in the method signatures and mocking, which was simply
+// a result of the evolution of the testutil package.
+// TODO_REFACTOR(@Olshansk): Rather than making `service`, `appAddr` and `supplierAddr`
+// explicit params, make them passable by the caller as options.
+func TokenomicsKeeperWithActorAddrs(
+	t testing.TB,
+	service *sharedtypes.Service,
+) (
 	tokenomicsKeeper tokenomicskeeper.Keeper,
 	ctx context.Context,
 	appAddr string,
@@ -197,7 +202,7 @@ func TokenomicsKeeperWithActorAddrs(t testing.TB, service *sharedtypes.Service)
 func NewTokenomicsModuleKeepers(
 	t testing.TB,
 	logger log.Logger,
-	opts ...TokenomicsKeepersOpt,
+	opts ...TokenomicsModuleKeepersOpt,
 ) (_ TokenomicsModuleKeepers, ctx context.Context) {
 	t.Helper()
 

testutil/testproxy/relayerproxy.go

@@ -156,7 +156,12 @@ func WithServicesConfigMap(
 				go func() {
 					err := server.ListenAndServe()
 					if err != nil && !errors.Is(err, http.ErrServerClosed) {
-						require.NoError(test.t, err)
+						require.NoError(test.t, err, `
+TODO_FLAKY: Known flaky test: 'TestRelayerProxy'
+
+Run the following command a few times to verify it passes at least once:
+
+$ go test -v -count=1 -run TestRelayerProxy ./pkg/relayer/proxy/...`)
 					}
 				}()
 

x/service/module/tx_add_service.go

@@ -17,7 +17,11 @@ import (
 
 var _ = strconv.Itoa(0)
 
-// TODO_MAINNET: Make it possible to update a service (e.g. update # of compute units per relay
+// TODO_UPNEXT: Change `add-service` to `update-service` so the source owner can
+// update the compute units per relay for an existing service. Make it possible
+// to update a service (e.g. update # of compute units per relay). This will require
+// search for all variations of `AddService` in the codebase (filenames, helpers, etc...),
+// ensuring that only the owner can update it on chain, and tackling some of the tests in `service.feature`.
 func CmdAddService() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   fmt.Sprintf("add-service <service_id> <service_name> [compute_units_per_relay: default={%d}]", types.DefaultComputeUnitsPerRelay),

x/service/types/message_add_service.go

@@ -9,8 +9,8 @@ import (
 const (
 	DefaultComputeUnitsPerRelay uint64 = 1
 	// ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service.
-	// TODO_MAINNET: The reason we have a maximum is to account for potential integer overflows. This is
-	// something that needs to be revisited or reconsidered prior to mainnet.
+	// TODO_MAINNET: The reason we have a maximum is to account for potential integer overflows.
+	// Should we revisit all uint64 and convert them to BigInts?
 	ComputeUnitsPerRelayMax uint64 = 2 ^ 16
 )
 

x/tokenomics/keeper/settle_session_accounting.go

@@ -6,12 +6,10 @@ import (
 
 	"cosmossdk.io/math"
 	cosmostypes "github.com/cosmos/cosmos-sdk/types"
+	"github.com/pokt-network/smt"
 
 	"github.com/pokt-network/poktroll/app/volatile"
 	"github.com/pokt-network/poktroll/pkg/crypto/protocol"
-
-	"github.com/pokt-network/smt"
-
 	"github.com/pokt-network/poktroll/telemetry"
 	apptypes "github.com/pokt-network/poktroll/x/application/types"
 	prooftypes "github.com/pokt-network/poktroll/x/proof/types"
@@ -106,7 +104,7 @@ func (k Keeper) SettleSessionAccounting(
 		return tokenomicstypes.ErrTokenomicsApplicationNotFound
 	}
 
-	computeUnitsPerRelay, err := k.getComputUnitsPerRelayFromApplication(application, sessionHeader.Service.Id)
+	computeUnitsPerRelay, err := k.getComputeUnitsPerRelayFromApplication(application, sessionHeader.Service.Id)
 	if err != nil {
 		return err
 	}
@@ -218,19 +216,24 @@ func (k Keeper) SettleSessionAccounting(
 	return nil
 }
 
-// relayCountToCoin calculates the amount of uPOKT to mint based on the number of relays, the service-specific ComputeUnitsPerRelay, and the ComputeUnitsPerTokenMultiplier tokenomics param
+// relayCountToCoin calculates the amount of uPOKT to mint based on the number of relays.
+// The service-specific ComputeUnitsPerRelay, and the global ComputeUnitsPerTokenMultiplier tokenomics params
+// are used for this calculation.
 func relayCountToCoin(numRelays, computeUnitsPerRelay uint64, computeUnitsToTokensMultiplier uint64) (cosmostypes.Coin, error) {
-	upokt := math.NewInt(int64(numRelays * computeUnitsPerRelay * computeUnitsToTokensMultiplier))
+	upoktAmount := math.NewInt(int64(numRelays * computeUnitsPerRelay * computeUnitsToTokensMultiplier))
 
-	if upokt.IsNegative() {
+	if upoktAmount.IsNegative() {
 		return cosmostypes.Coin{}, tokenomicstypes.ErrTokenomicsRootHashInvalid.Wrap("sum * compute_units_to_tokens_multiplier is negative")
 	}
 
-	return cosmostypes.NewCoin(volatile.DenomuPOKT, upokt), nil
+	return cosmostypes.NewCoin(volatile.DenomuPOKT, upoktAmount), nil
 }
 
-// getComputUnitsPerRelayFromApplication retrieves the ComputeUnitsPerRelay for a given service from the application's service configs
-func (k Keeper) getComputUnitsPerRelayFromApplication(application apptypes.Application, serviceID string) (cupr uint64, err error) {
+// getComputeUnitsPerRelayFromApplication retrieves the ComputeUnitsPerRelay for a given service from the application's service configs
+// TODO_REFACTOR: Rename this to getComputeUnitsPerRelayForService(serviceId) after
+// adding a dependency on the service module to the tokenomics module so it is cleaner
+// and more idiomatic, leveraging the `GetService` function directly. Would require updating logs below.
+func (k Keeper) getComputeUnitsPerRelayFromApplication(application apptypes.Application, serviceID string) (cupr uint64, err error) {
 	logger := k.Logger().With("method", "getComputeUnitsPerRelayFromApplication")
 
 	serviceConfigs := application.ServiceConfigs