Merge pull request #32 from 0xPolygon/mardizzone/POS-2693

Address TODOs

baseapp/block_gas_test.go

@@ -55,7 +55,7 @@ func (m BlockGasImpl) Set(ctx context.Context, msg *baseapptestutil.MsgKeyValue)
 }
 
 func TestBaseApp_BlockGas(t *testing.T) {
-	t.Skip("TODO HV2: fix and enable this test?")
+	t.Skip("TODO HV2: fix and enable this test")
 	testcases := []struct {
 		name         string
 		gasToConsume uint64 // gas to consume in the msg execution

crypto/keyring/keyring_ledger_test.go

@@ -56,7 +56,7 @@ func TestInMemoryCreateLedger(t *testing.T) {
 // TestSignVerify does some detailed checks on how we sign and validate
 // signatures
 func TestSignVerifyKeyRingWithLedger(t *testing.T) {
-	t.Skip("TODO HV2: fix and enable this test? Is ledger supported?")
+	t.Skip("TODO HV2: fix and enable this test")
 	dir := t.TempDir()
 	cdc := getCodec()
 
@@ -140,7 +140,7 @@ func TestAltKeyring_SaveLedgerKey(t *testing.T) {
 }
 
 func TestSignWithLedger(t *testing.T) {
-	t.Skip("TODO HV2: fix and enable this test? Is ledger supported?")
+	t.Skip("TODO HV2: fix and enable this test")
 	// Create two distinct Ledger records: recordA and recordB.
 	// RecordA is added to the Ledger but recordB is not added.
 	pathA := hd.NewFundraiserParams(0, types.CoinType, 0)

crypto/ledger/ledger_test.go

@@ -192,7 +192,7 @@ func getFakeTx(accountNumber uint32) []byte {
 }
 
 func TestSignaturesHD(t *testing.T) {
-	t.Skip("TODO HV2: enable and fix this test?")
+	t.Skip("TODO HV2: fix and enable this test")
 	for account := uint32(0); account < 100; account += 30 {
 		msg := getFakeTx(account)
 
@@ -212,7 +212,7 @@ func TestSignaturesHD(t *testing.T) {
 }
 
 func TestRealDeviceSecp256k1(t *testing.T) {
-	t.Skip("TODO HV2: enable and fix this test?")
+	t.Skip("TODO HV2: fix and enable this test")
 	msg := getFakeTx(50)
 	path := *hd.NewFundraiserParams(0, sdk.CoinType, 0)
 	priv, err := NewPrivKeySecp256k1Unsafe(path)

orm/encoding/ormkv/key_codec_test.go

@@ -16,7 +16,7 @@ import (
 )
 
 func TestKeyCodec(t *testing.T) {
-	t.Skip("TODO HV2: enable and fix this test?")
+	t.Skip("TODO HV2: fix and enable this test")
 	rapid.Check(t, func(t *rapid.T) {
 		key := testutil.TestKeyCodecGen(0, 5).Draw(t, "key")
 		for i := 0; i < 100; i++ {

proto/cosmos/gov/v1beta1/gov.proto

@@ -45,8 +45,6 @@ message WeightedVoteOption {
   ];
 }
 
-// TODO HV2: TextProposals are disabled in heimdall. If we remove it, we need to re-implement all tests. It should be ok as is.
-
 // TextProposal defines a standard text proposal whose changes need to be manually updated in case of approval.
 message TextProposal {
   option (cosmos_proto.implements_interface) = "cosmos.gov.v1beta1.Content";

server/util_test.go

@@ -421,7 +421,7 @@ func TestInterceptConfigsWithBadPermissions(t *testing.T) {
 }
 
 func TestEmptyMinGasPrices(t *testing.T) {
-	t.Skip("TODO HV2: enable and fix this test?")
+	t.Skip("TODO HV2: fix and enable this test")
 	tempDir := t.TempDir()
 	err := os.Mkdir(filepath.Join(tempDir, "config"), os.ModePerm)
 	require.NoError(t, err)

simapp/app_di.go

@@ -96,9 +96,6 @@ func init() {
 	}
 }
 
-// TODO HV2: To fix many tests, we need to implement https://polygon.atlassian.net/browse/POS-2540
-//  and change NewSimApp function accordingly
-
 // NewSimApp returns a reference to an initialized SimApp.
 func NewSimApp(
 	logger log.Logger,

tests/integration/bank/keeper/deterministic_test.go

@@ -187,7 +187,7 @@ func TestGRPCQueryAllBalances(t *testing.T) {
 }
 
 func TestGRPCQuerySpendableBalances(t *testing.T) {
-	t.Skip("TODO HV2: fix and enable this test?")
+	t.Skip("TODO HV2: fix and enable this test")
 	t.Parallel()
 	f := initDeterministicFixture(t)
 

tests/integration/distribution/keeper/msg_server_test.go

@@ -459,7 +459,7 @@ func TestMsgSetWithdrawAddress(t *testing.T) {
 }
 
 func TestMsgWithdrawValidatorCommission(t *testing.T) {
-	t.Skip("TODO HV2: fix and enable this test?")
+	t.Skip("TODO HV2: fix and enable this test")
 	t.Parallel()
 	f := initFixture(t)
 

tests/integration/tx/aminojson/aminojson_test.go

@@ -234,9 +234,7 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
 			},
 			roundTripUnequal: true,
 		},
-		/* TODO HV2: this fails because `keys.pulsar.go` doesn't have the new key changes
-		    from https://github.com/0xPolygon/cosmos-sdk/commits/mardizzone/POS-2511-auth/api/cosmos/crypto/secp256k1/keys.pulsar.go.
-		    Fix the tests based on https://polygon.atlassian.net/browse/POS-2730
+		/* TODO HV2: https://polygon.atlassian.net/browse/POS-2756
 		"auth/base_account": {
 			gogo:   &authtypes.BaseAccount{Address: addr1Str, PubKey: pubkeyAny},
 			pulsar: &authapi.BaseAccount{Address: addr1Str, PubKey: pubkeyAnyPulsar},
@@ -283,10 +281,7 @@ func TestAminoJSON_LegacyParity(t *testing.T) {
 			gogo:   &ed25519types.PubKey{Key: []byte("key")},
 			pulsar: &ed25519.PubKey{Key: []byte("key")},
 		},
-		/* TODO HV2: this succeeds because `keys.pulsar.go` doesn't have the new key changes
-		   from https://github.com/0xPolygon/cosmos-sdk/commits/mardizzone/POS-2511-auth/api/cosmos/crypto/secp256k1/keys.pulsar.go.
-		   Update the deps and fix the tests based on https://polygon.atlassian.net/browse/POS-2730
-		*/
+		// TODO HV2: https://polygon.atlassian.net/browse/POS-2756
 		"crypto/secp256k1": {
 			gogo:   &secp256k1types.PubKeyOld{Key: []byte("key")},
 			pulsar: &secp256k1.PubKey{Key: []byte("key")},

testutil/network/network.go

@@ -331,9 +331,6 @@ func NewCLILogger(cmd *cobra.Command) CLILogger {
 	return CLILogger{cmd}
 }
 
-// TODO HV2: To fix many tests, we need to implement https://polygon.atlassian.net/browse/POS-2540
-//  and change New function accordingly
-
 // New creates a new Network for integration tests or in-process testnets run via the CLI
 func New(l Logger, baseDir string, cfg Config) (*Network, error) {
 	// only one caller/test can create and use a network at a time

tools/confix/upgrade_test.go

@@ -19,7 +19,7 @@ func mustReadConfig(t *testing.T, path string) []byte {
 }
 
 func TestCheckValid(t *testing.T) {
-	t.Skip("TODO HV2: enable and fix this test?")
+	t.Skip("TODO HV2: fix and enable this test")
 	err := confix.CheckValid("foo", []byte{})
 	assert.ErrorContains(t, err, "unknown config")
 

x/auth/ante/ante.go

@@ -33,7 +33,7 @@ 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: confirm signing mode for heimdall is SignMode_SIGN_MODE_DIRECT
+	if options.SignModeHandler == nil {
 		return nil, errorsmod.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for ante builder")
 	}
 

x/auth/ante/ante_test.go

@@ -26,8 +26,6 @@ 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 related tests)
-
 // Test that simulate transaction accurately estimates gas cost
 func TestSimulateGasCost(t *testing.T) {
 	// This test has a test case that uses another's output.

x/auth/ante/sigverify.go

@@ -110,43 +110,6 @@ func (spkd SetPubKeyDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b
 			return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, err.Error())
 		}
 
-		/* TODO HV2: The following code is intended to comply with heimdall v1 (see https://polygon.atlassian.net/browse/POS-2492).
-		    There, `RecoverPubKey` is used to obtain the public key according to the eth-go library.
-		    This was done in x/auth/ante.go `processSig` method, which is now part of the `SigVerificationDecorator`.
-		    However, in v0.50.x, the `SigVerificationDecorator` expects the pubKey to be already set in `SetPubKeyDecorator`.
-		    So, this logic is now moved under `SetPubKeyDecorator`, which also requires a `signModeHandler` to invoke the `VerifySignature`.
-		    Such `VerifySignature` method has been modified to return the recovered public key, which is then set in the account.
-		    This code is hit every time pubKeys are set and txs (in `SigVerificationDecorator`) are verified, so it is very frequent.
-		    The code is currently disabled, because - if it was needed - secp256k1 pubkey.VerifySignature would return false already.
-		    If something breaks very early on in the game, we might need to revisit enable this code.
-		    More context: 	https://github.com/0xPolygon/cosmos-sdk/pull/3/#discussion_r1497996133
-		                    https://github.com/0xPolygon/cosmos-sdk/pull/3/#discussion_r1498023925
-
-		anyPk, _ := codectypes.NewAnyWithValue(pk)
-
-		signerData := txsigning.SignerData{
-			Address:       acc.GetAddress().String(),
-			ChainID:       ctx.ChainID(),
-			AccountNumber: acc.GetAccountNumber(),
-			Sequence:      acc.GetSequence(),
-			PubKey: &anypb.Any{
-				TypeUrl: anyPk.TypeUrl,
-				Value:   anyPk.Value,
-			},
-		}
-
-		adaptableTx, ok := tx.(authsigning.V2AdaptableTx)
-		if !ok {
-			return ctx, fmt.Errorf("expected tx to implement V2AdaptableTx, got %T", tx)
-		}
-		txData := adaptableTx.GetSigningTxData()
-		recoveredPubKey, err := authsigning.VerifySignature(ctx, pk, signerData, sigs[i].Data, spkd.signModeHandler, txData)
-		err = acc.SetPubKey(recoveredPubKey)
-		if err != nil {
-			return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, err.Error())
-		}
-		*/
-
 		spkd.ak.SetAccount(ctx, acc)
 	}
 
@@ -370,7 +333,7 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
 				return ctx, fmt.Errorf("expected tx to implement V2AdaptableTx, got %T", tx)
 			}
 			txData := adaptableTx.GetSigningTxData()
-			_, err := authsigning.VerifySignature(ctx, pubKey, signerData, sig.Data, svd.signModeHandler, txData)
+			err := authsigning.VerifySignature(ctx, pubKey, signerData, sig.Data, svd.signModeHandler, txData)
 
 			if err != nil {
 				var errMsg string

x/auth/client/cli/tx_multisign.go

@@ -163,7 +163,7 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) {
 				txData := adaptableTx.GetSigningTxData()
 
 				if !skipSigVerify {
-					_, err = signing.VerifySignature(cmd.Context(), sig.PubKey, txSignerData, sig.Data,
+					err = signing.VerifySignature(cmd.Context(), sig.PubKey, txSignerData, sig.Data,
 						txCfg.SignModeHandler(), txData)
 					if err != nil {
 						addr, _ := sdk.AccAddressFromHex(sig.PubKey.Address().String())
@@ -347,7 +347,7 @@ func makeBatchMultisignCmd() func(cmd *cobra.Command, args []string) error {
 			txData := adaptableTx.GetSigningTxData()
 
 			for _, sig := range signatureBatch {
-				_, err = signing.VerifySignature(cmd.Context(), sig[i].PubKey, txSignerData, sig[i].Data,
+				err = signing.VerifySignature(cmd.Context(), sig[i].PubKey, txSignerData, sig[i].Data,
 					txCfg.SignModeHandler(), txData)
 				if err != nil {
 					return fmt.Errorf("couldn't verify signature: %w %v", err, sig)

x/auth/client/cli/validate_sigs.go

@@ -148,7 +148,7 @@ func printAndValidateSigs(
 			}
 			txData := adaptableTx.GetSigningTxData()
 
-			_, err = authsigning.VerifySignature(cmd.Context(), pubKey, txSignerData, sig.Data, signModeHandler, txData)
+			err = authsigning.VerifySignature(cmd.Context(), pubKey, txSignerData, sig.Data, signModeHandler, txData)
 			if err != nil {
 				cmd.PrintErrf("failed to verify signature: %v", err)
 				return false

x/auth/keeper/deterministic_test.go

@@ -116,7 +116,7 @@ func (suite *DeterministicTestSuite) createAndSetAccounts(t *rapid.T, count int)
 }
 
 func (suite *DeterministicTestSuite) TestGRPCQueryAccount() {
-	suite.T().Skip("TODO HV2: enable and fix this test?")
+	suite.T().Skip("TODO HV2: fix and enable this test")
 	rapid.Check(suite.T(), func(t *rapid.T) {
 		accs := suite.createAndSetAccounts(t, 1)
 		req := &types.QueryAccountRequest{Address: accs[0].GetAddress().String()}

x/auth/signing/verify.go

@@ -2,6 +2,7 @@ package signing
 
 import (
 	"context"
+	"encoding/hex"
 	"fmt"
 
 	signingv1beta1 "cosmossdk.io/api/cosmos/tx/signing/v1beta1"
@@ -61,31 +62,27 @@ func internalSignModeToAPI(mode signing.SignMode) (signingv1beta1.SignMode, erro
 
 // VerifySignature verifies a transaction signature contained in SignatureData abstracting over different signing
 // modes. It differs from VerifySignature in that it uses the new txsigning.TxData interface in x/tx.
-func VerifySignature(ctx context.Context, pubKey cryptotypes.PubKey, signerData txsigning.SignerData, signatureData signing.SignatureData, handler *txsigning.HandlerMap, txData txsigning.TxData) (cryptotypes.PubKey, error) {
+func VerifySignature(ctx context.Context, pubKey cryptotypes.PubKey, signerData txsigning.SignerData, signatureData signing.SignatureData, handler *txsigning.HandlerMap, txData txsigning.TxData) error {
 	switch data := signatureData.(type) {
 	case *signing.SingleSignatureData:
 		signMode, err := internalSignModeToAPI(data.SignMode)
 		if err != nil {
-			return nil, err
+			return err
 		}
 		signBytes, err := handler.GetSignBytes(ctx, signMode, signerData, txData)
 		if err != nil {
-			return nil, err
+			return err
 		}
-		/* TODO HV2: See comment in `sigverify.go` (relates to https://polygon.atlassian.net/browse/POS-2492).
-		p, err := RecoverPubKey(signBytes, signatureData.(*signing.SingleSignatureData).Signature)
-		pubKey = &secp256k1.PubKey{Key: p}
-		*/
 		if !pubKey.VerifySignature(signBytes, data.Signature) {
-			return nil, fmt.Errorf("unable to verify single signer signature")
+			return fmt.Errorf("unable to verify single signer signature '%s' for signBytes '%s'", hex.EncodeToString(data.Signature), hex.EncodeToString(signBytes))
 		}
-		return pubKey, nil
+		return nil
 
 	case *signing.MultiSignatureData:
-		return nil, fmt.Errorf("MultiSignatureData not supported in heimdall")
+		return fmt.Errorf("MultiSignatureData not supported in heimdall")
 		multiPK, ok := pubKey.(multisig.PubKey)
 		if !ok {
-			return nil, fmt.Errorf("expected %T, got %T", (multisig.PubKey)(nil), pubKey)
+			return fmt.Errorf("expected %T, got %T", (multisig.PubKey)(nil), pubKey)
 		}
 		err := multiPK.VerifyMultisignature(func(mode signing.SignMode) ([]byte, error) {
 			signMode, err := internalSignModeToAPI(mode)
@@ -95,11 +92,11 @@ func VerifySignature(ctx context.Context, pubKey cryptotypes.PubKey, signerData
 			return handler.GetSignBytes(ctx, signMode, signerData, txData)
 		}, data)
 		if err != nil {
-			return nil, err
+			return err
 		}
-		return nil, nil
+		return nil
 	default:
-		return nil, fmt.Errorf("unexpected SignatureData %T", signatureData)
+		return fmt.Errorf("unexpected SignatureData %T", signatureData)
 	}
 }
 

x/bank/app_test.go

@@ -179,11 +179,10 @@ func TestSendNotEnoughBalance(t *testing.T) {
 			[]expectedBalance{
 				{addr3, sdk.Coins{sdk.NewInt64Coin("pol", defaultFeeAmount-1)}},
 
-				// TODO HV2: fee_collector's balance should be 2*defaultFeeAmount but since distribution module
-				// flushes the fees to the its module account at beginning of the block,
-				// the fee_collector's balance is 0.
-				// We should replace the native simapp with a modified version that imitates heimdall as much as possible
-				// to avoid such discrepancies.
+				// TODO HV2: https://polygon.atlassian.net/browse/POS-2540
+				//  fee_collector's balance should be 2*defaultFeeAmount but since distribution module
+				//  flushes the fees to the its module account at beginning of the block,
+				//  the fee_collector's balance is 0.
 				{s.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName), sdk.Coins{}},
 			},
 			false,
@@ -275,11 +274,10 @@ func TestMsgMultiSendWithAccounts(t *testing.T) {
 				{addr2, sdk.Coins{sdk.NewInt64Coin("pol", 10*defaultFeeAmount)}},
 				{addr3, sdk.Coins{}},
 
-				// TODO HV2: fee_collector's balance should be defaultFeeAmount but since distribution module
-				// flushes the fees to the its module account at beginning of the block,
-				// the fee_collector's balance is 0.
-				// We should replace the native simapp with a modified version that imitates heimdall as much as possible
-				// to avoid such discrepancies.
+				// TODO HV2: https://polygon.atlassian.net/browse/POS-2540:
+				//  fee_collector's balance should be defaultFeeAmount but since distribution module
+				//  flushes the fees to the its module account at beginning of the block,
+				//  the fee_collector's balance is 0.
 				{s.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName), sdk.Coins{}},
 			},
 		},
@@ -293,11 +291,10 @@ func TestMsgMultiSendWithAccounts(t *testing.T) {
 			privKeys:   []cryptotypes.PrivKey{priv1},
 			expectedBalances: []expectedBalance{
 				{addr1, sdk.Coins{sdk.NewInt64Coin("pol", 57*defaultFeeAmount)}},
-				// TODO HV2: fee_collector's balance should be defaultFeeAmount but since distribution module
-				// flushes the fees to the distribution module account at beginning of the block,
-				// the fee_collector's balance is 0.
-				// We should replace the native simapp with a modified version that imitates heimdall as much as possible
-				// to avoid such discrepancies.
+				// TODO HV2: https://polygon.atlassian.net/browse/POS-2540
+				//  fee_collector's balance should be defaultFeeAmount but since distribution module
+				//  flushes the fees to the distribution module account at beginning of the block,
+				//  the fee_collector's balance is 0.
 				{s.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName), sdk.Coins{}},
 			},
 		},
@@ -313,11 +310,10 @@ func TestMsgMultiSendWithAccounts(t *testing.T) {
 				{addr1, sdk.Coins{sdk.NewInt64Coin("pol", 57*defaultFeeAmount)}},
 				{addr2, sdk.Coins{sdk.NewInt64Coin("pol", 10*defaultFeeAmount)}},
 
-				// TODO HV2: fee_collector's balance should be defaultFeeAmount but since distribution module
-				// flushes the fees to the distribution module account at beginning of the block,
-				// the fee_collector's balance is 0.
-				// We should replace the native simapp with a modified version that imitates heimdall as much as possible
-				// to avoid such discrepancies.
+				// TODO HV2: https://polygon.atlassian.net/browse/POS-2540
+				//  fee_collector's balance should be defaultFeeAmount but since distribution module
+				//  flushes the fees to the distribution module account at beginning of the block,
+				//  the fee_collector's balance is 0.
 				{s.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName), sdk.Coins{}},
 			},
 		},
@@ -436,11 +432,10 @@ func TestMsgMultiSendDependent(t *testing.T) {
 				{addr1, sdk.Coins{sdk.NewInt64Coin("pol", 42*defaultFeeAmount)}},
 				{addr2, sdk.Coins{}},
 
-				// TODO HV2: fee_collector's balance should be 2*defaultFeeAmount but since distribution module
-				// flushes the fees to the distribution module account at beginning of the block,
-				// the fee_collector's balance is 0.
-				// We should replace the native simapp with a modified version that imitates heimdall as much as possible
-				// to avoid such discrepancies.
+				// TODO HV2: https://polygon.atlassian.net/browse/POS-2540
+				//  fee_collector's balance should be 2*defaultFeeAmount but since distribution module
+				//  flushes the fees to the distribution module account at beginning of the block,
+				//  the fee_collector's balance is 0.
 				{s.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName), sdk.Coins{sdk.NewInt64Coin("pol", defaultFeeAmount)}},
 			},
 		},

x/gov/README.md

@@ -139,8 +139,6 @@ according to the final tally of the proposal. In Heimdall, burn is not enabled,
 Polygon PoS network, participants are validators. Other holders and users do not get the right to participate in governance. 
 However, they can submit and deposit on proposals.
 
-[//]: # ( TODO HV2: if we don't want non-validators to submit/deposit on proposals, we would need to add an additional check then on gov proposal submission, to validate the proposer is in the active set)
-
 Note that for *participants*, their voting power is calculated from their L1 POL stakes only.
 
 #### Voting period