chg: more meaningful comments for the upcoming PR reviews

simapp/app.go

@@ -401,7 +401,7 @@ func NewSimApp(
 			app.AccountKeeper, app.StakingKeeper, app,
 			txConfig,
 		),
-		// TODO HV2 init processors with proper supply.AccountProcessor
+		// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 		auth.NewAppModule(appCodec, app.AccountKeeper, authsims.RandomGenesisAccounts, app.GetSubspace(authtypes.ModuleName), []authtypes.AccountProcessor{}),
 		vesting.NewAppModule(app.AccountKeeper, app.BankKeeper),
 		bank.NewAppModule(appCodec, app.BankKeeper, app.AccountKeeper, app.GetSubspace(banktypes.ModuleName)),
@@ -507,7 +507,7 @@ func NewSimApp(
 	//
 	// NOTE: this is not required apps that don't use the simulator for fuzz testing
 	// transactions
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	overrideModules := map[string]module.AppModuleSimulation{
 		authtypes.ModuleName: auth.NewAppModule(app.appCodec, app.AccountKeeper, authsims.RandomGenesisAccounts, app.GetSubspace(authtypes.ModuleName), []authtypes.AccountProcessor{}),
 	}

simapp/app_v2.go

@@ -243,7 +243,7 @@ func NewSimApp(
 	//
 	// NOTE: this is not required apps that don't use the simulator for fuzz testing
 	// transactions
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	overrideModules := map[string]module.AppModuleSimulation{
 		authtypes.ModuleName: auth.NewAppModule(app.appCodec, app.AccountKeeper, authsims.RandomGenesisAccounts, app.GetSubspace(authtypes.ModuleName), []authtypes.AccountProcessor{}),
 	}

simapp/go.mod

@@ -215,6 +215,6 @@ replace (
 	// replace broken goleveldb
 	github.com/syndtr/goleveldb => github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7
 
-	// TODO HV2 this fixes the build temporarily > to remove once we have our version of the cosmos-sdk fork
+	// TODO HV2: this fixes the app build temporarily > to remove once we have our version of the cosmos-sdk fork
 	cosmossdk.io/x/evidence => ../x/evidence
 )

tests/integration/bank/keeper/deterministic_test.go

@@ -98,7 +98,7 @@ func initDeterministicFixture(t *testing.T) *deterministicFixture {
 		authority.String(),
 		log.NewNopLogger(),
 	)
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	authModule := auth.NewAppModule(cdc, accountKeeper, authsims.RandomGenesisAccounts, nil, []authtypes.AccountProcessor{})
 	bankModule := bank.NewAppModule(cdc, bankKeeper, accountKeeper, nil)
 

tests/integration/distribution/keeper/msg_server_test.go

@@ -103,7 +103,7 @@ func initFixture(t testing.TB) *fixture {
 	distrKeeper := distrkeeper.NewKeeper(
 		cdc, runtime.NewKVStoreService(keys[distrtypes.StoreKey]), accountKeeper, bankKeeper, stakingKeeper, distrtypes.ModuleName, authority.String(),
 	)
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	authModule := auth.NewAppModule(cdc, accountKeeper, authsims.RandomGenesisAccounts, nil, []authtypes.AccountProcessor{})
 	bankModule := bank.NewAppModule(cdc, bankKeeper, accountKeeper, nil)
 	stakingModule := staking.NewAppModule(cdc, stakingKeeper, accountKeeper, bankKeeper, nil)

tests/integration/evidence/keeper/infraction_test.go

@@ -128,7 +128,7 @@ func initFixture(t testing.TB) *fixture {
 	router := evidencetypes.NewRouter()
 	router = router.AddRoute(evidencetypes.RouteEquivocation, testEquivocationHandler(evidenceKeeper))
 	evidenceKeeper.SetRouter(router)
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	authModule := auth.NewAppModule(cdc, accountKeeper, authsims.RandomGenesisAccounts, nil, []authtypes.AccountProcessor{})
 	bankModule := bank.NewAppModule(cdc, bankKeeper, accountKeeper, nil)
 	stakingModule := staking.NewAppModule(cdc, stakingKeeper, accountKeeper, bankKeeper, nil)

tests/integration/gov/keeper/keeper_test.go

@@ -123,7 +123,7 @@ func initFixture(t testing.TB) *fixture {
 	govKeeper.SetLegacyRouter(govRouter)
 	err = govKeeper.Params.Set(newCtx, v1.DefaultParams())
 	assert.NilError(t, err)
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	authModule := auth.NewAppModule(cdc, accountKeeper, authsims.RandomGenesisAccounts, nil, []authtypes.AccountProcessor{})
 	bankModule := bank.NewAppModule(cdc, bankKeeper, accountKeeper, nil)
 	stakingModule := staking.NewAppModule(cdc, stakingKeeper, accountKeeper, bankKeeper, nil)

tests/integration/staking/keeper/common_test.go

@@ -134,7 +134,7 @@ func initFixture(t testing.TB) *fixture {
 	)
 
 	stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewKVStoreService(keys[types.StoreKey]), accountKeeper, bankKeeper, authority.String(), addresscodec.NewHexCodec(sdk.Bech32PrefixValAddr), addresscodec.NewHexCodec(sdk.Bech32PrefixConsAddr))
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	authModule := auth.NewAppModule(cdc, accountKeeper, authsims.RandomGenesisAccounts, nil, []authtypes.AccountProcessor{})
 	bankModule := bank.NewAppModule(cdc, bankKeeper, accountKeeper, nil)
 	stakingModule := staking.NewAppModule(cdc, stakingKeeper, accountKeeper, bankKeeper, nil)

tests/integration/staking/keeper/determinstic_test.go

@@ -107,7 +107,7 @@ func initDeterministicFixture(t *testing.T) *deterministicFixture {
 	)
 
 	stakingKeeper := stakingkeeper.NewKeeper(cdc, runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), accountKeeper, bankKeeper, authority.String(), addresscodec.NewHexCodec(sdk.Bech32PrefixValAddr), addresscodec.NewHexCodec(sdk.Bech32PrefixConsAddr))
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	authModule := auth.NewAppModule(cdc, accountKeeper, authsims.RandomGenesisAccounts, nil, []authtypes.AccountProcessor{})
 	bankModule := bank.NewAppModule(cdc, bankKeeper, accountKeeper, nil)
 	stakingModule := staking.NewAppModule(cdc, stakingKeeper, accountKeeper, bankKeeper, nil)

testutil/integration/example_test.go

@@ -51,7 +51,7 @@ func Example() {
 	)
 
 	// subspace is nil because we don't test params (which is legacy anyway)
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	authModule := auth.NewAppModule(encodingCfg.Codec, accountKeeper, authsims.RandomGenesisAccounts, nil, []authtypes.AccountProcessor{})
 
 	// here bankkeeper and staking keeper is nil because we are not testing them
@@ -141,7 +141,7 @@ func Example_oneModule() {
 	)
 
 	// subspace is nil because we don't test params (which is legacy anyway)
-	// TODO HV2 init processors with proper supply.AccountProcessor
+	// TODO HV2: init processors with proper supply.AccountProcessor (supply module has been merged with bank module upstream)
 	authModule := auth.NewAppModule(encodingCfg.Codec, accountKeeper, authsims.RandomGenesisAccounts, nil, []authtypes.AccountProcessor{})
 
 	// create the application and register all the modules from the previous step

types/address.go

@@ -19,9 +19,6 @@ import (
 )
 
 const (
-
-	// TODO HV2 prefixes are now all empty strings. Do we need Purpose, CoinType, FullFundraiserPath ?
-
 	// Constants defined here are the defaults value for address.
 	// You can use the specific values for your project.
 	// Add the follow lines to the `main()` of your server.
@@ -34,6 +31,11 @@ const (
 	//	config.SetCoinType(yourCoinType)
 	//	config.Seal()
 
+	// TODO HV2: all prefixes are now empty strings.
+	//  Another solution (closer to what's being done in cosmos-sdk) would be to use "0x" as our prefix.
+	//  This would require changes to the codecs
+	//  Also, our app won't use `Purpose`, `CoinType` and `FullFundraiserPath`. Kept them for compatibility only.
+
 	// Bech32MainPrefix defines the main SDK Bech32 prefix of an account's address
 	Bech32MainPrefix = ""
 
@@ -348,7 +350,8 @@ func (va ValAddress) String() string {
 	if va.Empty() {
 		return ""
 	}
-	// TODO HV2: is the 0x addition to be done for all Tx related types? Like TxHash. In case, is there a way to do it with proto?
+	// TODO HV2: the "0x" addition is preserved from heimdall and necessary to have eth addresses string representations
+	//  Does it need to be done for all `Tx` related types? Like `TxHash`
 	return "0x" + common.Bytes2Hex(va.Bytes())
 }
 

x/auth/ante/ante.go

@@ -19,19 +19,19 @@ type HandlerOptions struct {
 	SignModeHandler        *txsigning.HandlerMap
 	SigGasConsumer         func(meter storetypes.GasMeter, sig signing.SignatureV2, params types.Params) error
 	TxFeeChecker           TxFeeChecker
-	// TODO HV2 is FeeCollector == FeegrantKeeper?
-	// TODO HV2: heimdall is using this for the supply method `SendCoinsFromAccountToModule`.
-	//  Upstream supply is merged with bank, so do we need this, or BankKeeper is enough?
+	// TODO HV2: is FeeCollector == FeegrantKeeper?
+	//  Can our business logic be implemented by using the FeegrantKeeper and avoid additional changes?
+	//  Also, heimdall uses FeeCollector for its interface method `SendCoinsFromAccountToModule`.
+	//  Since supply module is merged with bank module upstream, is BankKeeper's method enough?
 	FeeCollector FeeCollector
 }
 
+// TODO HV2: We need to double check the NewAnteHandler function and all the Decorators it uses (especially the AnteHandle methods of each Decorator).
+//  Also, is this implementation enough or we need further changes to reconcile with heimdall's `auth/ante.go` ?
+
 // NewAnteHandler returns an AnteHandler that checks and increments sequence
 // numbers, checks signatures & account numbers, and deducts fees from the first
 // signer.
-
-// TODO HV2 double check this function and all the decorators
-//
-//	is this enough to reconcile with heimdall's auth/ante.go?
 func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
 	if options.AccountKeeper == nil {
 		return nil, errorsmod.Wrap(sdkerrors.ErrLogic, "account keeper is required for ante builder")
@@ -41,12 +41,10 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
 		return nil, errorsmod.Wrap(sdkerrors.ErrLogic, "bank keeper is required for ante builder")
 	}
 
-	if options.SignModeHandler == nil { // TODO HV2: what is the signing mode for heimdall? SignDoc?
+	if options.SignModeHandler == nil { // TODO HV2: what is the signing mode for heimdall? Is it `SignDoc` ?
 		return nil, errorsmod.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for ante builder")
 	}
 
-	// TODO HV2: heimdall is using this for the supply method `SendCoinsFromAccountToModule`.
-	//  Upstream supply is merged with bank, so do we need this, or BankKeeper is enough?
 	if options.FeeCollector == nil {
 		return nil, errorsmod.Wrap(sdkerrors.ErrLogic, "fee collector has not been set")
 	}
@@ -55,12 +53,12 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
 		NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first
 		NewExtensionOptionsDecorator(options.ExtensionOptionChecker),
 		NewValidateBasicDecorator(),
-		// NewTxTimeoutHeightDecorator(), // TODO HV2 this is not present in heimdall
-		NewValidateMemoDecorator(options.AccountKeeper), // TODO HV2 can we keep this despite we don't support Memo?
-		// NewConsumeGasForTxSizeDecorator(options.AccountKeeper), // TODO HV2 this was removed in heimdall's auth/ante.go (original ancestor's method `newCtx.GasMeter().ConsumeGas`)
-		NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker, options.FeeCollector), // TODO HV2 heavily changed, double check
+		// NewTxTimeoutHeightDecorator(), // TODO HV2: this is not present in heimdall. Is it safe to remove?
+		NewValidateMemoDecorator(options.AccountKeeper), // TODO HV2: can we keep this despite we don't support Memo? Or is it better/safer to remove it?
+		// NewConsumeGasForTxSizeDecorator(options.AccountKeeper), // TODO HV2: this was removed in heimdall's `auth/ante.go` (original ancestor's method `newCtx.GasMeter().ConsumeGas`)
+		NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker, options.FeeCollector), // TODO HV2: heavily changed, double check
 		NewSetPubKeyDecorator(options.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators
-		// NewValidateSigCountDecorator(options.AccountKeeper), // TODO HV2 this was removed in heimdall's auth/ante.go (original ancestor's method `ValidateSigCount`)
+		// NewValidateSigCountDecorator(options.AccountKeeper), // TODO HV2: this was removed in heimdall's `auth/ante.go` (original ancestor's method `ValidateSigCount`)
 		NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer),
 		NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler),
 		NewIncrementSequenceDecorator(options.AccountKeeper),

x/auth/ante/ante_test.go

@@ -26,7 +26,7 @@ import (
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 )
 
-// TODO HV2: reconcile this file with heimdall's auth/ante_test.go (e.g. implement heimdall specific tests like TestCheckpointGasLimit and milestones tests)
+// TODO HV2: reconcile this file with heimdall's `auth/ante_test.go` (e.g. implement heimdall specific tests like `TestCheckpointGasLimit` and milestones related tests)
 
 // Test that simulate transaction accurately estimates gas cost
 func TestSimulateGasCost(t *testing.T) {
@@ -524,7 +524,7 @@ func TestAnteHandlerFees(t *testing.T) {
 
 // Test logic around memo gas consumption.
 func TestAnteHandlerMemoGas(t *testing.T) {
-	t.Skip() // TODO HV2 skipped as we don't support memo
+	t.Skip() // TODO HV2: skipped as we don't support memo. To be fixed?
 	testCases := []TestCase{
 		{
 			"tx does not have enough gas",
@@ -1215,7 +1215,7 @@ func TestCustomSignatureVerificationGasConsumer(t *testing.T) {
 }
 
 func TestAnteHandlerReCheck(t *testing.T) {
-	t.Skip() // TODO HV2 fix this test?
+	t.Skip() // TODO HV2: This test is currently broken. To be fixed?
 	suite := SetupTestSuite(t, false)
 	// Set recheck=true
 	suite.ctx = suite.ctx.WithIsReCheckTx(true)

x/auth/ante/fee.go

@@ -88,7 +88,7 @@ func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee
 	// feeGranter := feeTx.FeeGranter()
 	deductFeesFrom := feePayer
 
-	// TODO HV2: removed freeGranter logic as we are not using it in heimdall. Is this ok? Do we want to use it instead of FeeCollector?
+	// TODO HV2: removed `FeeGranter` logic as we are not using it in heimdall. Is this ok? Do we want to use it instead of `FeeCollector`?
 	// if feegranter set deduct fee from feegranter account.
 	// this works with only when feegrant enabled.
 	//if feeGranter != nil {

x/auth/ante/fee_test.go

@@ -50,7 +50,7 @@ func TestDeductFeeDecorator_ZeroGas(t *testing.T) {
 }
 
 func TestEnsureMempoolFees(t *testing.T) {
-	t.Skip()                     // TODO HV2 skipped as we use a different fee model. Double check
+	t.Skip()                     // TODO HV2: skipped as we use a different fee model. To be fixed?
 	s := SetupTestSuite(t, true) // setup
 	s.txBuilder = s.clientCtx.TxConfig.NewTxBuilder()
 

x/auth/ante/sigverify.go

@@ -38,7 +38,9 @@ var (
 	// DefaultFeeInMatic represents default fee in matic
 	DefaultFeeInMatic = big.NewInt(10).Exp(big.NewInt(10), big.NewInt(15), nil)
 
-	// TODO HV2 no usage of DefaultFeeWantedPerTx so far. This is used in heimdall topup module's side_handler.go
+	// TODO HV2: no usage of DefaultFeeWantedPerTx so far.
+	//  This is used in heimdall topup module's `side_handler.go`
+
 	// DefaultFeeWantedPerTx fee wanted per tx
 	DefaultFeeWantedPerTx = sdk.Coins{sdk.Coin{Denom: types.FeeToken, Amount: math.NewIntFromBigInt(DefaultFeeInMatic)}}
 )
@@ -55,7 +57,8 @@ func init() {
 // This is where apps can define their own PubKey
 type SignatureVerificationGasConsumer = func(meter storetypes.GasMeter, sig signing.SignatureV2, params types.Params) error
 
-// TODO HV2 double check this and its usage
+// TODO HV2: MainTxMsg has no usage so far. Double check it by looking at heimdall's code.
+
 // MainTxMsg tx hash
 type MainTxMsg interface {
 	GetTxHash() address.HeimdallHash
@@ -197,7 +200,7 @@ func (sgcd SigGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 		return ctx, err
 	}
 
-	// TODO HV2 following checks have been enforced due to heimdall use case
+	// TODO HV2: following checks have been enforced due to heimdall business logic (we don't support multisig yet)
 	if len(signers) == 0 {
 		return ctx, sdkerrors.ErrNoSignatures
 	}
@@ -277,7 +280,8 @@ func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool {
 }
 
 func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
-	// TODO HV2 do we need any change here?
+
+	// TODO HV2: do we need any change here to be compliant with heimdall's business logic?
 
 	sigTx, ok := tx.(authsigning.Tx)
 	if !ok {

x/auth/ante/validator_tx_fee.go

@@ -19,11 +19,11 @@ func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx, params type
 		return nil, 0, errorsmod.Wrap(sdkerrors.ErrInvalidTxFees, "must provide correct txFees")
 	}
 
-	// TODO HV2: imported from heimdall
+	// TODO HV2: gas is retrieved from Params as currently done in heimdall. Can this be changed
 	gas := params.GetMaxTxGas()
 	feeCoins := sdk.Coins{sdk.Coin{Denom: types.FeeToken, Amount: amount}}
 
-	// TODO HV2: removed as not present in heimdall
+	// TODO HV2: removed as not present in heimdall. Is this safe? Would it be better/safer to implement it?
 	// Ensure that the provided fees meet a minimum threshold for the validator,
 	// if this is a CheckTx. This is only for local mempool purposes, and thus
 	// is only ran on check tx.

x/auth/client/cli/broadcast.go

@@ -11,7 +11,8 @@ import (
 	authclient "github.com/cosmos/cosmos-sdk/x/auth/client"
 )
 
-// TODO HV2 import helper and apply changes from heimdall's client/tx/broadcast.go
+// TODO HV2: GetBroadcastCommand is slightly different in heimdall.
+//  We need import the `helper` and apply the changes from heimdall's `client/tx/broadcast.go`
 
 // GetBroadcastCommand returns the tx broadcast command.
 func GetBroadcastCommand() *cobra.Command {

x/auth/client/cli/encode.go

@@ -10,7 +10,8 @@ import (
 	authclient "github.com/cosmos/cosmos-sdk/x/auth/client"
 )
 
-// TODO HV2 import helper and apply changes from heimdall's client/tx/broadcast.go
+// TODO HV2: GetEncodeCommand is slightly different in heimdall.
+//  We need import the `helper` and apply the changes from heimdall's `client/tx/broadcast.go`
 
 // GetEncodeCommand returns the encode command to take a JSONified transaction and turn it into
 // Amino-serialized bytes

x/auth/keeper/deterministic_test.go

@@ -146,7 +146,7 @@ func pubkeyGenerator(t *rapid.T) *rapid.Generator[secp256k1.PubKey] {
 }
 
 func (suite *DeterministicTestSuite) TestGRPCQueryAccounts() {
-	// suite.T().Skip() // TODO HV2 skipped as it uses depinject
+	suite.T().Skip()
 	rapid.Check(suite.T(), func(t *rapid.T) {
 		numAccs := rapid.IntRange(1, 10).Draw(t, "accounts")
 		accs := suite.createAndSetAccounts(t, numAccs)
@@ -199,7 +199,7 @@ func (suite *DeterministicTestSuite) TestGRPCQueryAccountAddressByID() {
 }
 
 func (suite *DeterministicTestSuite) TestGRPCQueryParameters() {
-	// suite.T().Skip() // TODO HV2 skipped as we use a different fee model
+	// suite.T().Skip() // TODO HV2: skipped as we use a different fee model. To be fixed?
 	rapid.Check(suite.T(), func(t *rapid.T) {
 		params := types.NewParams(
 			rapid.Uint64Min(1).Draw(t, "max-memo-characters"),

x/auth/keeper/genesis.go

@@ -29,7 +29,7 @@ func (ak AccountKeeper) InitGenesis(ctx sdk.Context, data types.GenesisState, pr
 			lastAccNum = &n
 		}
 
-		// TODO HV2 imported from heimdall
+		// TODO HV2: this is imported from heimdall
 		baseAcc := types.NewBaseAccount(acc.GetAddress(), acc.GetPubKey(), accNum, acc.GetSequence())
 
 		// execute account processors