fix(*): resolve and close todos (#426)

address open TODOs

issue: piplabs/trust-story#19

.github/workflows/ci-foundry.yml

@@ -38,13 +38,7 @@ jobs:
       - run: pnpm install
         working-directory: contracts
 
-      # Run lint
-      # TODO: Fix and unify linting
-      # - name: Check lint
-      #  run: pnpm lint-check
-      #  working-directory: contracts
-
-      # first, build contracts excluding the tests and scripts. Check contract sizes in this step.
+      # Build contracts excluding the tests and scripts. Check contract sizes in this step.
       - name: Run Contract Size check
         run: |
           forge --version

.pre-commit/run_go_tests.sh

@@ -4,5 +4,4 @@
 MOD=$(go list -m)
 PKGS=$(echo "$@"| xargs -n1 dirname | sort -u | sed -e "s#^#${MOD}/#")
 
-# TODO: fix tests and enable
-# go test -tags=verify_logs -failfast -race -timeout=2m $PKGS
+go test -tags=verify_logs -failfast -race -timeout=5m $PKGS

client/app/app.go

@@ -180,7 +180,6 @@ func (App) SimulationManager() *module.SimulationManager {
 }
 
 // SetCometAPI sets the comet API client.
-// TODO: Figure out how to use depinject to set this.
 func (a App) SetCometAPI(api comet.API) {
 	a.Keepers.EVMEngKeeper.SetCometAPI(api)
 }

client/app/start.go

@@ -13,6 +13,7 @@ import (
 	cmtcfg "github.com/cometbft/cometbft/config"
 	"github.com/cometbft/cometbft/node"
 	"github.com/cometbft/cometbft/p2p"
+	"github.com/cometbft/cometbft/privval"
 	"github.com/cometbft/cometbft/proxy"
 	rpclocal "github.com/cometbft/cometbft/rpc/client/local"
 	cmttypes "github.com/cometbft/cometbft/types"
@@ -91,45 +92,11 @@ func Start(ctx context.Context, cfg Config) (func(context.Context) error, error)
 		return nil, errors.Wrap(err, "enable cosmos-sdk telemetry")
 	}
 
-	privVal, err := loadPrivVal(cfg)
-	if err != nil {
-		return nil, errors.Wrap(err, "load validator key")
-	}
-
-	db, err := dbm.NewDB("application", cfg.BackendType(), cfg.DataDir())
-	if err != nil {
-		return nil, errors.Wrap(err, "create db")
-	}
-
-	baseAppOpts, err := makeBaseAppOpts(cfg)
-	if err != nil {
-		return nil, errors.Wrap(err, "make base app opts")
-	}
-
-	engineCl, err := newEngineClient(ctx, cfg)
+	app, privVal, err := CreateApp(ctx, cfg)
 	if err != nil {
 		return nil, err
 	}
 
-	//nolint:contextcheck // False positive
-	app, err := newApp(
-		newSDKLogger(ctx),
-		db,
-		engineCl,
-		baseAppOpts...,
-	)
-	if err != nil {
-		return nil, errors.Wrap(err, "create app")
-	}
-	app.Keepers.EVMEngKeeper.SetBuildDelay(cfg.EVMBuildDelay)
-	app.Keepers.EVMEngKeeper.SetBuildOptimistic(cfg.EVMBuildOptimistic)
-
-	addr, err := k1util.PubKeyToAddress(privVal.Key.PrivKey.PubKey())
-	if err != nil {
-		return nil, errors.Wrap(err, "convert validator pubkey to address")
-	}
-	app.Keepers.EVMEngKeeper.SetValidatorAddress(addr)
-
 	cmtNode, err := newCometNode(ctx, &cfg.Comet, app, privVal)
 	if err != nil {
 		return nil, errors.Wrap(err, "create comet node")
@@ -185,26 +152,25 @@ func Start(ctx context.Context, cfg Config) (func(context.Context) error, error)
 	}, nil
 }
 
-// TODO: Refactor CreateApp() to be used within the Start function, as most of the code originates from there.
-func CreateApp(ctx context.Context, cfg Config) *App {
+func CreateApp(ctx context.Context, cfg Config) (*App, *privval.FilePV, error) {
 	privVal, err := loadPrivVal(cfg)
 	if err != nil {
-		panic(errors.Wrap(err, "load validator key"))
+		return nil, nil, errors.Wrap(err, "load validator key")
 	}
 
 	db, err := dbm.NewDB("application", cfg.BackendType(), cfg.DataDir())
 	if err != nil {
-		panic(errors.Wrap(err, "create db"))
+		return nil, nil, errors.Wrap(err, "create db")
 	}
 
 	baseAppOpts, err := makeBaseAppOpts(cfg)
 	if err != nil {
-		panic(errors.Wrap(err, "make base app opts"))
+		return nil, nil, errors.Wrap(err, "make base app opts")
 	}
 
 	engineCl, err := newEngineClient(ctx, cfg)
 	if err != nil {
-		panic(err)
+		return nil, nil, errors.Wrap(err, "create engine client")
 	}
 
 	//nolint:contextcheck // False positive
@@ -215,18 +181,18 @@ func CreateApp(ctx context.Context, cfg Config) *App {
 		baseAppOpts...,
 	)
 	if err != nil {
-		panic(errors.Wrap(err, "create app"))
+		return nil, nil, errors.Wrap(err, "create app")
 	}
 	app.Keepers.EVMEngKeeper.SetBuildDelay(cfg.EVMBuildDelay)
 	app.Keepers.EVMEngKeeper.SetBuildOptimistic(cfg.EVMBuildOptimistic)
 
 	addr, err := k1util.PubKeyToAddress(privVal.Key.PrivKey.PubKey())
 	if err != nil {
-		panic(errors.Wrap(err, "convert validator pubkey to address"))
+		return nil, nil, errors.Wrap(err, "convert validator pubkey to address")
 	}
 	app.Keepers.EVMEngKeeper.SetValidatorAddress(addr)
 
-	return app
+	return app, privVal, nil
 }
 
 func newCometNode(ctx context.Context, cfg *cmtcfg.Config, app *App, privVal cmttypes.PrivValidator,
@@ -286,7 +252,7 @@ func makeBaseAppOpts(cfg Config) ([]func(*baseapp.BaseApp), error) {
 	}
 
 	return []func(*baseapp.BaseApp){
-		// baseapp.SetOptimisticExecution(), // TODO: Enable this.
+		// baseapp.SetOptimisticExecution(), // TODO: research this feature
 		baseapp.SetChainID(chainID),
 		baseapp.SetMinRetainBlocks(cfg.MinRetainBlocks),
 		baseapp.SetPruning(pruneOpts),

client/cmd/rollback.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/cometbft/cometbft/privval"
 	"github.com/spf13/cobra"
 
 	"github.com/piplabs/story/client/app"
@@ -13,7 +14,7 @@ import (
 )
 
 // newRollbackCmd returns a new cobra command that rolls back one block of the story consensus client.
-func newRollbackCmd(appCreateFunc func(context.Context, app.Config) *app.App) *cobra.Command {
+func newRollbackCmd(appCreateFunc func(context.Context, app.Config) (*app.App, *privval.FilePV, error)) *cobra.Command {
 	rollbackCfg := cfg.DefaultRollbackConfig()
 	storyCfg := cfg.DefaultConfig()
 	logCfg := log.DefaultConfig()
@@ -46,7 +47,10 @@ CometBFT the transactions in blocks [n - X + 1, n] will be re-executed against t
 				Config: storyCfg,
 				Comet:  cometCfg,
 			}
-			a := appCreateFunc(ctx, appCfg)
+			a, _, err := appCreateFunc(ctx, appCfg)
+			if err != nil {
+				return err
+			}
 			lastHeight, lastHash, err := app.RollbackCometAndAppState(a, cometCfg, rollbackCfg)
 			if err != nil {
 				return err

client/collections/queue.go

@@ -159,7 +159,6 @@ func (q Queue[T]) Rear(ctx context.Context) (uint64, error) {
 	return item, nil
 }
 
-// TODO: Iterate with a custom range, clamp the range to the front and rear.
 func (q Queue[T]) Iterate(ctx context.Context) (collections.Iterator[uint64, T], error) {
 	front, _ := q.front.Get(ctx)
 	rear, _ := q.rear.Get(ctx)

client/genutil/evm/predeploys/predeploys.go

@@ -58,21 +58,10 @@ func Alloc(_ netconf.ID) (types.GenesisAlloc, error) {
 
 	db := state.NewMemDB(emptyGenesis)
 
-	// setProxies(db)
-
-	// admin, err := eoa.Admin(network)
-	// if err != nil {
-	//	return nil, errors.Wrap(err, "network admin")
-	//}
-
 	if err := setStaking(db); err != nil {
 		return nil, errors.Wrap(err, "set staking")
 	}
 
-	if err := setSlashing(db); err != nil {
-		return nil, errors.Wrap(err, "set slashing")
-	}
-
 	return db.Genesis().Alloc, nil
 }
 
@@ -83,14 +72,6 @@ func setStaking(db *state.MemDB) error {
 	return setPredeploy(db, ipTokenStaking, ipTokenStakingCode, bindings.IPTokenStakingStorageLayout, storage)
 }
 
-// setSlashing sets the Slashing predeploy.
-// TODO: Slashing design.
-func setSlashing(db *state.MemDB) error {
-	storage := state.StorageValues{}
-
-	return setPredeploy(db, ubiPool, ipTokenStakingCode, bindings.IPTokenStakingStorageLayout, storage)
-}
-
 // setPredeploy sets the implementation code and proxy storage for the given predeploy.
 func setPredeploy(db *state.MemDB, proxy common.Address, code []byte, layout solc.StorageLayout, storage state.StorageValues) error {
 	impl := impl(proxy)

client/genutil/genutil.go

@@ -237,7 +237,6 @@ func defaultAppState(
 }
 
 func getCodec() *codec.ProtoCodec {
-	// TODO: Use depinject to get all of this.
 	sdkConfig := sdk.GetConfig()
 	reg, err := codectypes.NewInterfaceRegistryWithOptions(codectypes.InterfaceRegistryOptions{
 		ProtoFiles: proto.HybridResolver,

client/proto/story/evmstaking/v1/types/evmstaking.proto

@@ -17,16 +17,11 @@ message Withdrawal {
   option (gogoproto.goproto_getters) = false;
 
   uint64 creation_height = 1;
-  // TODO: use ethcommon.Address type
   string execution_address = 2 [
     (cosmos_proto.scalar) = "cosmos.AddressString",
     (gogoproto.moretags) = "yaml:\"execution_address\""
   ];
   uint64 amount = 3 [
-    // TODO: use custom Int type, need to resolve issue in auto-generated pb.go
-    // (cosmos_proto.scalar)  = "cosmos.Int",
-    // (gogoproto.customtype) = "cosmossdk.io/math.Int",
-    // (gogoproto.nullable) = false,
     (gogoproto.moretags) = "yaml:\"amount\""
   ];
   WithdrawalType withdrawal_type = 4 [

client/proto/story/evmstaking/v1/types/genesis.proto

@@ -8,16 +8,11 @@ option go_package = "github.com/piplabs/story/client/x/evmstaking/types";
 
 message GenesisState {
   Params params = 1 [(gogoproto.nullable) = false];
-  // TODO: Add withdrawals collections field as ORM if needed
   ValidatorSweepIndex validator_sweep_index = 2 [(gogoproto.nullable) = false];
 }
 
 message ValidatorSweepIndex {
   uint64 next_val_index = 1 [
-    // TODO: use custom Int type, need to resolve issue in auto-generated pb.go
-    // (cosmos_proto.scalar)  = "cosmos.Int",
-    // (gogoproto.customtype) = "cosmossdk.io/math.Int",
-    // (gogoproto.nullable) = false,
     (gogoproto.moretags) = "yaml:\"next_val_index\""
   ];
   uint64 next_val_del_index = 2 [

client/x/evmengine/keeper/abci.go

@@ -202,7 +202,6 @@ func (k *Keeper) PostFinalize(ctx sdk.Context) error {
 	logAttr := slog.Int64("next_height", nextHeight)
 	log.Info(ctx, "Starting optimistic EVM payload build", logAttr)
 
-	// TODO: This will fail because ctx provided in ABCI call in CometBFT 0.37 is context.TODO()
 	// If optimistic block is discarded, we need to restore the dequeued withdrawals back to the queue.
 	// Thus, we peek here. If this optimistic block is indeed used in the next block, then we dequeue there.
 	// If this block is not used in the next block, the block itself is discarded and the queue is unaffected.
@@ -211,7 +210,7 @@ func (k *Keeper) PostFinalize(ctx sdk.Context) error {
 		log.Error(ctx, "Starting optimistic build failed; get max withdrawal", err, logAttr)
 		return errors.Wrap(err, "get max withdrawal per block")
 	}
-	withdrawals, err := k.evmstakingKeeper.PeekEligibleWithdrawals(ctx, maxWithdrawals) // context is "context.TODO()"" (empty) in CometBFT v0.38
+	withdrawals, err := k.evmstakingKeeper.PeekEligibleWithdrawals(ctx, maxWithdrawals)
 	if err != nil {
 		log.Error(ctx, "Starting optimistic build failed; withdrawals peek", err, logAttr)
 		return nil
@@ -300,7 +299,6 @@ func isUnknownPayload(err error) bool {
 		return false
 	}
 
-	// TODO: Add support for typed errors.
 	if strings.Contains(
 		strings.ToLower(err.Error()),
 		strings.ToLower(engine.UnknownPayload.Error()),

client/x/evmengine/keeper/genesis.go

@@ -36,7 +36,6 @@ func (k *Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
 	}
 }
 
-//nolint:revive // TODO: validate genesis
-func (k *Keeper) ValidateGenesis(gs *types.GenesisState) error {
+func (*Keeper) ValidateGenesis(gs *types.GenesisState) error {
 	return types.ValidateExecutionBlockHash(gs.Params.ExecutionBlockHash)
 }

client/x/evmengine/keeper/msg_server.go

@@ -37,14 +37,6 @@ func (s msgServer) ExecutionPayload(ctx context.Context, msg *types.MsgExecution
 		return nil, err
 	}
 
-	// TODO: should we compare and reject in a finalized block?
-	//// Ensure that the withdrawals in the payload are from the front indices of the queue.
-	// if err := s.compareWithdrawals(ctx, payload.Withdrawals); err != nil {
-	//	return nil, errors.Wrap(err, "compare local and received withdrawals")
-	//}
-
-	// TODO: We dequeue with assumption that the top items of the queue are the ones that are processed in the block.
-	// TODO: We might need to check that the withdrawals in the finalized block are the same as the ones dequeued.
 	// Since we already checked the withdrawals in the proposal server, we simply check the length here.
 	log.Debug(
 		ctx, "Dequeueing eligible withdrawals [BEFORE]",

client/x/evmengine/keeper/proposal_server.go

@@ -73,7 +73,6 @@ func NewProposalServer(keeper *Keeper) types.MsgServiceServer {
 
 var _ types.MsgServiceServer = proposalServer{}
 
-// TODO: benchmark this function, might be adding to overhead. Esp. the for loop. If so, parallelize since array checks are independent.
 func evmEventsEqual(a, b []*types.EVMEvent) error {
 	if len(a) != len(b) {
 		return errors.New("count mismatch")

client/x/evmstaking/keeper/abci.go

@@ -15,7 +15,6 @@ import (
 
 // Query staking module's UnbondingDelegation (UBD Queue) to get the matured unbonding delegations. Then,
 // insert the matured unbonding delegations into the withdrawal queue.
-// TODO: check if unbonded delegations in staking module must be distinguished based on source of generation, CL or EL.
 func (k *Keeper) EndBlock(ctx context.Context) (abci.ValidatorUpdates, error) {
 	log.Debug(ctx, "EndBlock.evmstaking")
 	defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker)

client/x/evmstaking/keeper/deposit.go

@@ -122,15 +122,13 @@ func (k Keeper) ProcessDeposit(ctx context.Context, ev *bindings.IPTokenStakingD
 		delID = stypes.FlexiblePeriodDelegationID
 	}
 
-	// TODO: Check if we can instantiate the msgServer without type assertion
 	evmstakingSKeeper, ok := k.stakingKeeper.(*skeeper.Keeper)
 	if !ok {
 		return errors.New("type assertion failed")
 	}
 
 	// Note that, after minting, we save the mapping between delegator bech32 address and evm address, which will be used in the withdrawal queue.
 	// The saving is done regardless of any error below, as the money is already minted and sent to the delegator, who can withdraw the minted amount.
-	// TODO: Confirm that bech32 address and evm address can be used interchangeably. Must be one-to-one or many-bech32-to-one-evm.
 	// NOTE: Do not overwrite the existing withdraw/reward address set by the delegator.
 	if exists, err := k.DelegatorWithdrawAddress.Has(cachedCtx, depositorAddr.String()); err != nil {
 		return errors.Wrap(err, "check delegator withdraw address existence")

client/x/evmstaking/keeper/genesis.go

@@ -91,8 +91,7 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
 	}
 }
 
-//nolint:revive // TODO: validate genesis
-func (k Keeper) ValidateGenesis(gs *types.GenesisState) error {
+func (Keeper) ValidateGenesis(gs *types.GenesisState) error {
 	if err := gs.Params.Validate(); err != nil {
 		return errors.Wrap(err, "validate genesis state params")
 	}

client/x/evmstaking/keeper/grpc_query.go

@@ -31,7 +31,6 @@ func (k Keeper) Params(ctx context.Context, request *types.QueryParamsRequest) (
 }
 
 // GetWithdrawalQueue returns the withdrawal queue in pagination.
-// TODO: GetWithdrawalQueue tests.
 func (k Keeper) GetWithdrawalQueue(ctx context.Context, request *types.QueryGetWithdrawalQueueRequest) (*types.QueryGetWithdrawalQueueResponse, error) {
 	if request == nil {
 		return nil, status.Error(codes.InvalidArgument, "empty request")

client/x/evmstaking/keeper/utils.go

@@ -9,8 +9,7 @@ import (
 )
 
 // IPTokenToBondCoin converts the IP amount into a $STAKE coin.
-// TODO: At this point, it is 1-to1, but this might change in the future.
-// TODO: parameterized bondDenom.
+// NOTE: Assumes that the only bondable token is $STAKE on CL (using IP token).
 func IPTokenToBondCoin(amount *big.Int) (sdk.Coin, sdk.Coins) {
 	coin := sdk.NewCoin(sdk.DefaultBondDenom, math.NewIntFromBigInt(amount))
 	return coin, sdk.NewCoins(coin)

client/x/evmstaking/keeper/validator.go

@@ -90,7 +90,6 @@ func (k Keeper) ProcessCreateValidator(ctx context.Context, ev *bindings.IPToken
 		"amount_coin", amountCoin.String(),
 	)
 
-	// TODO: Check if we can instantiate the msgServer without type assertion
 	evmstakingSKeeper, ok := k.stakingKeeper.(*skeeper.Keeper)
 	if !ok {
 		return errors.New("type assertion failed")
@@ -135,7 +134,6 @@ func (k Keeper) ProcessCreateValidator(ctx context.Context, ev *bindings.IPToken
 
 	// Note that, after minting, we save the mapping between delegator bech32 address and evm address, which will be used in the withdrawal queue.
 	// The saving is done regardless of any error below, as the money is already minted and sent to the delegator, who can withdraw the minted amount.
-	// TODO: Confirm that bech32 address and evm address can be used interchangeably. Must be one-to-one or many-bech32-to-one-evm.
 	// NOTE: Do not overwrite the existing withdraw/reward address set by the validator.
 	if exists, err := k.DelegatorWithdrawAddress.Has(cachedCtx, delegatorAddr.String()); err != nil {
 		return errors.Wrap(err, "check delegator withdraw address existence")