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

[TODOs] Update #2 to TODO_BETA to only reflect ACTUAL blockers #900

Merged
merged 1 commit into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions cmd/poktrolld/cmd/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,21 +156,21 @@ func customPoktrollAppConfigTemplate() string {
###############################################################################
### Poktroll ###
###############################################################################

# Poktroll-specific app configuration for Full Nodes and Validators.
[poktroll]

# Telemetry configuration in addition to the [telemetry] settings.
[poktroll.telemetry]

# Cardinality level for telemetry metrics collection
# This controls the level of detail (number of unique labels) in metrics.
# Options:
# - "low": Collects basic metrics with low cardinality.
# - "low": Collects basic metrics with low cardinality.
# Suitable for production environments with tight performance constraints.
# - "medium": Collects a moderate number of labels, balancing detail and performance.
# Suitable for moderate workloads or staging environments.
# - "high": WARNING: WILL CAUSE STRESS TO YOUR MONITORING ENVIRONMENT! Collects detailed metrics with high
# - "high": WARNING: WILL CAUSE STRESS TO YOUR MONITORING ENVIRONMENT! Collects detailed metrics with high
# cardinality, including labels with many unique values (e.g., application_id, session_id).
# Recommended for debugging or testing environments.
cardinality-level = "{{ .Poktroll.Telemetry.CardinalityLevel }}"
Expand Down
2 changes: 1 addition & 1 deletion cmd/poktrolld/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import (
relayercmd "github.com/pokt-network/poktroll/pkg/relayer/cmd"
)

// TODO_MAINNET: adjust chain ID for the mainnet if it's going to change
// TODO_MAINNET: adjust chain ID to `pocket`, `pokt` or `shannon`
const DefaultChainID = "poktroll"

// NewRootCmd creates a new root command for poktrolld. It is called once in the main function.
Expand Down
12 changes: 5 additions & 7 deletions config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ validators:
app:
# DEV_NOTE: Ignite does not carry over all defaults, so we are going to match `minimum-gas-prices` with `cmd/config.go`.
# See the enhancement request here: https://github.com/ignite/cli/issues/4340
# TODO_MAINNET(#794): turn on `minimum-gas-prices` back
# TODO_BETA(@okdas, #794): turn on `minimum-gas-prices` back
# minimum-gas-prices: 0.000000001upokt
telemetry:
enabled: true
Expand Down Expand Up @@ -128,9 +128,7 @@ genesis:
staking:
params:
bond_denom: upokt
# TODO_MAINNET: Figure out what this should be on Shannon
# re-genesis. We're setting it to 1 for Alpha TestNet #1 so Grove
# maintains the only validator until Alpha TestNet #2.
# TODO_BETA(@okdas): Update this to 10 for Beta TestNet.
max_validators: 1
crisis:
constant_fee:
Expand Down Expand Up @@ -170,7 +168,7 @@ genesis:
params:
max_delegated_gateways: "7"
min_stake:
# TODO_MAINNET: Determine realistic amount for minimum application stake amount.
# TODO_BETA(@bryanchriswhite): Determine realistic amount for minimum application stake amount.
amount: "100000000" # 100 POKT
denom: upokt
applicationList:
Expand All @@ -196,7 +194,7 @@ genesis:
denom: upokt
supplier:
params:
# TODO_MAINNET: Determine realistic amount for minimum gateway stake amount.
# TODO_BETA(@bryanchriswhite): Determine realistic amount for minimum gateway stake amount.
min_stake:
amount: "1000000" # 1 POKT
denom: upokt
Expand Down Expand Up @@ -235,7 +233,7 @@ genesis:
denom: upokt
gateway:
params:
# TODO_MAINNET: Determine realistic amount for minimum gateway stake amount.
# TODO_BETA(@bryanchriswhite): Determine realistic amount for minimum gateway stake amount.
min_stake:
amount: "1000000" # 1 POKT
denom: upokt
Expand Down
70 changes: 34 additions & 36 deletions makefiles/todos.mk
Original file line number Diff line number Diff line change
@@ -1,45 +1,47 @@
#############
### TODOS ###
#############

# How do I use TODOs?
# 1. <KEYWORD>: <Description of follow up work>;
## Inspired by @goldinguy_ in this post: https://goldin.io/blog/stop-using-todo ###
#
## How do I use TODOs?
#
# 1. TODO_<KEYWORD>: <Description of the work todo and why>
# e.g. TODO_HACK: This is a hack, we need to fix it later
# 2. If there's a specific issue, or specific person, add that in paranthesiss
#
# 2. If there's a specific issue and/or person, include it in paranthesiss
#
# e.g. TODO(@Olshansk): Automatically link to the Github user https://github.com/olshansk
# e.g. TODO_INVESTIGATE(#420): Automatically link this to github issue https://github.com/pokt-network/poktroll/issues/420
# e.g. TODO_DISCUSS(@Olshansk, #420): Specific individual should tend to the action item in the specific ticket
# e.g. TODO_CLEANUP(core): This is not tied to an issue, or a person, but should only be done by the core team.
# e.g. TODO_CLEANUP: This is not tied to an issue, or a person, and can be done by the core team or external contributors.
#
# 3. Feel free to add additional keywords to the list above.

# Inspired by @goldinguy_ in this post: https://goldin.io/blog/stop-using-todo ###
# TODO - General Purpose catch-all.
#
## TODO LIST
# TODO - General Purpose catch-all. Try to keep the usage of this to a minimum.
# TODO_COMMUNITY - A TODO that may be a candidate for outsourcing to the community.
# TODO_DECIDE - A TODO indicating we need to make a decision and document it using an ADR in the future; https://github.com/pokt-network/pocket-network-protocol/tree/main/ADRs
# TODO_TECHDEBT - Not a great implementation, but we need to fix it later.
# TODO_BLOCKER - BEFORE MAINNET. Similar to TECHDEBT, but of higher priority, urgency & risk prior to the next release
# TODO_QOL - AFTER MAINNET. Similar to TECHDEBT, but of lower priority. Doesn't deserve a GitHub Issue but will improve everyone's life.
# TODO_IMPROVE - A nice to have, but not a priority. It's okay if we never get to this.
# TODO_OPTIMIZE - An opportunity for performance improvement if/when it's necessary
# TODO_DISCUSS - Probably requires a lengthy offline discussion to understand next steps.
# TODO_INCOMPLETE - A change which was out of scope of a specific PR but needed to be documented.
# TODO_INVESTIGATE - TBD what was going on, but needed to continue moving and not get distracted.
# TODO_CLEANUP - Like TECHDEBT, but not as bad. It's okay if we never get to this.
# TODO_HACK - Like TECHDEBT, but much worse. This needs to be prioritized
# TODO_REFACTOR - Similar to TECHDEBT, but will require a substantial rewrite and change across the codebase
# TODO_CONSIDERATION - A comment that involves extra work but was thoughts / considered as part of some implementation
# TODO_CONSOLIDATE - We likely have similar implementations/types of the same thing, and we should consolidate them.
# TODO_ADDTEST / TODO_TEST - Add more tests for a specific code section
# TODO_FLAKY - Signals that the test is flaky and we are aware of it. Provide an explanation if you know why.
# TODO_DEPRECATE - Code that should be removed in the future
# TODO_RESEARCH - A non-trivial action item that requires deep research and investigation being next steps can be taken
# TODO_DOCUMENT - A comment that involves the creation of a README or other documentation
# TODO_BUG - There is a known existing bug in this code
# TODO_NB - An important note to reference later
# TODO_DISCUSS_IN_THIS_COMMIT - SHOULD NEVER BE COMMITTED TO MASTER. It is a way for the reviewer of a PR to start / reply to a discussion.
# TODO_IN_THIS_COMMIT - SHOULD NEVER BE COMMITTED TO MASTER. It is a way to start the review process while non-critical changes are still in progress

# TODO_TECHDEBT - Code that works but isn’t ideal; needs a fix to improve maintainability and avoid accumulating technical debt.
# TODO_QOL - AFTER MAINNET. Low-priority improvements that enhance the code quality or developer experience.
# TODO_IMPROVE - A nice-to-have but not a priority; it’s okay if we never get to this.
# TODO_OPTIMIZE - Performance improvements that can be pursued if performance demands increase.
# TODO_DISCUSS - Marks code that requires a larger discussion with the team to clarify next steps or make decisions.
# TODO_INCOMPLETE - Notes unfinished work or partial implementation that should be completed later.
# TODO_INVESTIGATE - Requires more investigation to clarify issues or validate behavior; avoid premature optimizations without research.
# TODO_CLEANUP - Lower-priority cleanup or refactoring tasks; it’s acceptable if this is never prioritized.
# TODO_HACK - Code is functional but particularly bad; prioritization needed to fix due to the hacky implementation.
# TODO_REFACTOR - Indicates a need for a significant rewrite or architectural change across the codebase.
# TODO_CONSIDERATION - Marks optional considerations or ideas related to the code that could be explored later.
# TODO_CONSOLIDATE - Similar implementations or data structures are likely scattered; consolidate to reduce duplication.
# TODO_TEST / TODO_ADDTEST - Signals that additional test coverage is needed in this code section.
# TODO_FLAKY - Known flaky test; provides an explanation if there’s an understanding of the root cause.
# TODO_DEPRECATE - Marks code slated for eventual removal or replacement.
# TODO_RESEARCH - Requires substantial research or exploration before proceeding with next steps or optimization.
# TODO_DOCUMENT - Involves creating or updating documentation, such as READMEs, inline comments, or other resources.
# TODO_BUG - Known bug exists; this should be prioritized based on severity.
# TODO_NB - Important note that may not require immediate action but should be referenced later.
# TODO_IN_THIS_??? - Indicates ongoing non-critical changes before final review. THIS SHOULD NEVER BE COMMITTED TO MASTER and has workflows to prevent it.
Olshansk marked this conversation as resolved.
Show resolved Hide resolved
# TODO_UPNEXT(@???) - Indicates this should be done shortly after an existing PR. THIS MUST HAVE A USER ASSIGNED TO IT and has workflows to prevent it.

# Define shared variable for the exclude parameters
EXCLUDE_GREP = --exclude-dir={.git,vendor,./docusaurus,.vscode,.idea} --exclude={Makefile,reviewdog.yml,*.pb.go,*.pulsar.go}
Expand All @@ -50,8 +52,4 @@ todo_list: ## List all the TODOs in the project (excludes vendor and prototype d

.PHONY: todo_count
todo_count: ## Print a count of all the TODOs in the project
grep -r $(EXCLUDE_GREP) TODO . | grep -v 'TODO()' | wc -l

.PHONY: todo_this_commit
todo_this_commit: ## List all the TODOs needed to be done in this commit
grep -r $(EXCLUDE_GREP) TODO_IN_THIS .| grep -v 'TODO()'
grep -r $(EXCLUDE_GREP) TODO . | grep -v 'TODO()' | wc -l
2 changes: 1 addition & 1 deletion pkg/appgateserver/config/appgate_configs_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
)

// YAMLAppGateServerConfig is the structure used to unmarshal the AppGateServer config file
// TODO_BETA(@red-0ne): Rename self_signing parameter to `sovereign` in code, configs
// TODO_MAINNET(@red-0ne): Rename self_signing parameter to `sovereign` in code, configs
// and documentation
type YAMLAppGateServerConfig struct {
ListeningEndpoint string `yaml:"listening_endpoint"`
Expand Down
2 changes: 1 addition & 1 deletion pkg/relayer/session/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestRelayerSessionsManager_ColdStartRelayMinerWithUnclaimedRelays(t *testin
// along with its dependencies before starting it.
// It takes in the proofParams to configure the proof requirements and the proofCount
// to assert the number of proofs to be requested.
// TODO_BETA(@red-0ne): Add a test case which verifies that the service's compute units per relay is used as
// TODO_MAINNET(@red-0ne): Add a test case which verifies that the service's compute units per relay is used as
// the weight of a relay when updating a session's SMT.
func requireProofCountEqualsExpectedValueFromProofParams(t *testing.T, proofParams prooftypes.Params, proofCount int) {
var (
Expand Down
2 changes: 1 addition & 1 deletion pkg/relayer/session/sessiontree.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ var _ relayer.SessionTree = (*sessionTree)(nil)
// the number of requests that an application can pay for. This needs to be tracked
// based on the app's stake in the beginning of a session and the number of nodes
// per session. An operator should be able to specify "overservicing_compute_units_limit"
// whereby an upper bound on how much it can overservice an application is set. The
// whereby an upper bound on how much it can overserviced an application is set. The
// default value for this should be -1, implying "unlimited".
// Ref discussion: https://github.com/pokt-network/poktroll/pull/755#discussion_r1737287860
type sessionTree struct {
Expand Down
2 changes: 1 addition & 1 deletion proto/poktroll/application/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ message Application {
// - https://github.com/pokt-network/poktroll/pull/750#discussion_r1735025033
// - https://www.notion.so/buildwithgrove/Off-chain-Application-Stake-Tracking-6a8bebb107db4f7f9dc62cbe7ba555f7
repeated poktroll.shared.ApplicationServiceConfig service_configs = 3; // The list of services this appliccation is configured to request service for
// TODO_BETA: Rename `delegatee_gateway_addresses` to `gateway_addresses_delegated_to`.
// TODO_BETA(@bryanchriswhite): Rename `delegatee_gateway_addresses` to `gateway_addresses_delegated_to`.
// Ensure to rename all relevant configs, comments, variables, function names, etc as well.
repeated string delegatee_gateway_addresses = 4 [(cosmos_proto.scalar) = "cosmos.AddressString", (gogoproto.nullable) = false]; // The Bech32 encoded addresses for all delegatee Gateways, in a non-nullable slice
// A map from sessionEndHeights to a list of Gateways.
Expand Down
2 changes: 1 addition & 1 deletion proto/poktroll/service/relay_mining_difficulty.proto
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ message RelayMiningDifficulty {
// 0b0000111... (until 32 bytes are filled up).
bytes target_hash = 4;

// TODO_BETA(@Olshansk): Add a `hash_algorithm` field either in this
// TODO_MAINNET(@bryanchriswhite): Add a `hash_algorithm` field either in this
// structure or elsewhere so we can support changing it over time. There should
// be one source of truth, somewhere on chain, to stay in sync with the SMT
// configuration.
Expand Down
1 change: 0 additions & 1 deletion proto/poktroll/service/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ message MsgUpdateParamResponse {
// MsgAddService defines a message for adding a new message to the network.
// Services can be added by any actor in the network making them truly
// permissionless.
// TODO_BETA: Add Champions / Sources once its fully defined.
message MsgAddService {
option (cosmos.msg.v1.signer) = "owner_address"; // https://docs.cosmos.network/main/build/building-modules/messages-and-queries
string owner_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the service owner.
Expand Down
2 changes: 1 addition & 1 deletion proto/poktroll/shared/service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ message Service {
// For example, what if we want to request a session for a certain service but with some additional configs that identify it?
string id = 1; // Unique identifier for the service

// TODO_BETA: Either remove this or rename it to alias.
// TODO_BETA(@bryanchriswhite): Either remove this or rename it to alias.
string name = 2; // (Optional) Semantic human readable name for the service

// The cost of a single relay for this service in terms of compute units.
Expand Down
5 changes: 2 additions & 3 deletions x/proof/keeper/msg_server_submit_proof.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,7 @@ func (k Keeper) ProofRequirementForClaim(ctx context.Context, claim *types.Claim
}

// Require a proof if the claim's compute units meets or exceeds the threshold.
// TODO_BETA(@red-0ne): Should the threshold be dependant on the stake as well
// so we slash proportional to the compute units?
// TODO_BETA(@olshansk): Should the threshold be dependant on the stake as well so we slash proportional to the compute units?
// TODO_BETA(@red-0ne): It might make sense to include whether there was a proof
// submission error downstream from here. This would require a more comprehensive metrics API.
if claimeduPOKT.Amount.GTE(proofParams.GetProofRequirementThreshold().Amount) {
Expand Down Expand Up @@ -301,7 +300,7 @@ func (k Keeper) getProofRequirementSeedBlockHash(
proofWindowOpenHeight := sharedtypes.GetProofWindowOpenHeight(sharedParams, sessionEndHeight)
proofWindowOpenBlockHash := k.sessionKeeper.GetBlockHash(ctx, proofWindowOpenHeight)

// TODO_BETA(@red-0ne): Update the method header of this function to accept (sharedParams, Claim, BlockHash).
// TODO_TECHDEBT(@red-0ne): Update the method header of this function to accept (sharedParams, Claim, BlockHash).
// After doing so, please review all calling sites and simplify them accordingly.
earliestSupplierProofCommitHeight := sharedtypes.GetEarliestSupplierProofCommitHeight(
sharedParams,
Expand Down
2 changes: 1 addition & 1 deletion x/session/keeper/session_hydrator.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ import (

var SHA3HashLen = crypto.SHA3_256.Size()

// TODO_BETA(@bryanchriswhite): Make this a governance parameter
const (
// TODO_BETA(@bryanchriswhite): Make this a governance parameter
NumSupplierPerSession = 15
)

Expand Down
7 changes: 3 additions & 4 deletions x/supplier/keeper/msg_server_stake_supplier.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@ import (
)

var (
// TODO_BETA: Make supplier staking fee a governance parameter
// TODO_BETA(@red-0ne): Update supplier staking documentation to remove the upstaking
// requirement and introduce the staking fee.
// TODO_BETA(@bryanchriswhite): Make supplier staking fee a governance parameter
// TODO_BETA(@red-0ne): Update supplier staking documentation to remove the upstaking requirement and introduce the staking fee.
SupplierStakingFee = sdk.NewInt64Coin(volatile.DenomuPOKT, 1)
)

Expand Down Expand Up @@ -112,7 +111,7 @@ func (k msgServer) StakeSupplier(ctx context.Context, msg *types.MsgStakeSupplie
supplier.UnstakeSessionEndHeight = sharedtypes.SupplierNotUnstaking
}

// TODO_BETA: Remove requirement of MUST ALWAYS stake or upstake (>= 0 delta)
// TODO_BETA(@red-0ne): Remove requirement of MUST ALWAYS stake or upstake (>= 0 delta)
// TODO_POST_MAINNET: Should we allow stake decrease down to min stake?
if coinsToEscrow.IsNegative() {
err = types.ErrSupplierInvalidStake.Wrapf(
Expand Down
4 changes: 2 additions & 2 deletions x/tokenomics/keeper/settle_pending_claims.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ func (k Keeper) SettlePendingClaims(ctx sdk.Context) (

// If the proof is missing or invalid -> expire it
if expirationReason != tokenomicstypes.ClaimExpirationReason_EXPIRATION_REASON_UNSPECIFIED {
// TODO_BETA(@red-0ne): Slash the supplier in proportion
// to their stake. Consider allowing suppliers to RemoveClaim via a new
// TODO_BETA(@red-0ne): Slash the supplier in proportion to their stake.
// TODO_POST_MAINNET: Consider allowing suppliers to RemoveClaim via a new
// message in case it was sent by accident

// Proof was required but is invalid or not found.
Expand Down
Loading
Loading