Merge pull request #518 from PeggyJV/collin/small-refactors

Small refactors

integration_tests/validator_out.go

@@ -106,35 +106,6 @@ func (s *IntegrationTestSuite) TestValidatorOut() {
 			return true
 		}, 5*time.Minute, 10*time.Second, "unable to send to ethereum")
 
-		// Create Transaction batch
-		s.Require().Eventuallyf(func() bool {
-			batchTx := types.NewMsgRequestBatchTx(gravityDenom, s.chain.validators[2].keyInfo.GetAddress())
-
-			keyRing, err := s.chain.validators[2].keyring()
-			s.Require().NoError(err)
-
-			clientCtx, err := s.chain.clientContext("tcp://localhost:26657", &keyRing, "val", s.chain.validators[2].keyInfo.GetAddress())
-			s.Require().NoError(err)
-
-			response, err := s.chain.sendMsgs(*clientCtx, batchTx)
-			s.T().Logf("batch response: %s", response)
-			if err != nil {
-				s.T().Logf("error: %s", err)
-				return false
-			}
-
-			if response.Code != 0 {
-				if response.Code != 32 {
-					s.T().Log(response)
-				}
-				return false
-			}
-
-			s.Require().NoError(err, "error querying delegator bonded validators")
-
-			return true
-		}, 5*time.Minute, 1*time.Second, "can't create TX batch successfully")
-
 		// Confirm batchtx signatures
 		s.Require().Eventuallyf(func() bool {
 			keyRing, err := s.chain.validators[3].keyring()

module/proto/gravity/v1/msgs.proto

@@ -19,9 +19,6 @@ service Msg {
       returns (MsgCancelSendToEthereumResponse) {
     // option (google.api.http).post = "/gravity/v1/send_to_ethereum/cancel";
   }
-  rpc RequestBatchTx(MsgRequestBatchTx) returns (MsgRequestBatchTxResponse) {
-    // option (google.api.http).post = "/gravity/v1/batchtx/request";
-  }
   rpc SubmitEthereumTxConfirmation(MsgSubmitEthereumTxConfirmation)
       returns (MsgSubmitEthereumTxConfirmationResponse) {
     // option (google.api.http).post = "/gravity/v1/ethereum_signature";
@@ -53,7 +50,7 @@ message MsgSendToEthereum {
 // will be included in the batch tx.
 message MsgSendToEthereumResponse { uint64 id = 1; }
 
-// MsgCancelSendToEthereum allows the sender to cancel its own outgoing
+// MsgCancelSendToEthereum allows the sender to cancel its own unbatched
 // SendToEthereum tx and recieve a refund of the tokens and bridge fees. This tx
 // will only succeed if the SendToEthereum tx hasn't been batched to be
 // processed and relayed to Ethereum.
@@ -64,15 +61,6 @@ message MsgCancelSendToEthereum {
 
 message MsgCancelSendToEthereumResponse {}
 
-// MsgRequestBatchTx requests a batch of transactions with a given coin
-// denomination to send across the bridge to Ethereum.
-message MsgRequestBatchTx {
-  string denom = 1;
-  string signer = 2;
-}
-
-message MsgRequestBatchTxResponse {}
-
 // MsgSubmitEthereumTxConfirmation submits an ethereum signature for a given
 // validator
 message MsgSubmitEthereumTxConfirmation {

module/x/gravity/abci.go

@@ -48,7 +48,7 @@ func createBatchTxs(ctx sdk.Context, k keeper.Keeper) {
 
 		for _, c := range contracts {
 			// NOTE: this doesn't emit events which would be helpful for client processes
-			k.BuildBatchTx(ctx, common.HexToAddress(c), 100)
+			k.CreateBatchTx(ctx, common.HexToAddress(c), 100)
 		}
 	}
 }
@@ -155,11 +155,11 @@ func eventVoteRecordTally(ctx sdk.Context, k keeper.Keeper) {
 // order to keep this information current regardless of the level of bridge activity.
 //
 // We determine if we should update the latest heights based on the following criteria:
-// 1. A consensus of validators agrees that the proposed height is equal to or less than their
-//    last observed height, in order to reconcile the many different heights that will be submitted.
-//    The highest height that meets this criteria will be the proposed height.
-// 2. The proposed consensus heights from this process are greater than the values stored from the last time
-//    we observed an Ethereum event from the bridge
+//  1. A consensus of validators agrees that the proposed height is equal to or less than their
+//     last observed height, in order to reconcile the many different heights that will be submitted.
+//     The highest height that meets this criteria will be the proposed height.
+//  2. The proposed consensus heights from this process are greater than the values stored from the last time
+//     we observed an Ethereum event from the bridge
 func updateObservedEthereumHeight(ctx sdk.Context, k keeper.Keeper) {
 	// wait some minutes before checking the height votes
 	if ctx.BlockHeight()%50 != 0 {
@@ -228,12 +228,14 @@ func updateObservedEthereumHeight(ctx sdk.Context, k keeper.Keeper) {
 // cleanupTimedOutBatchTxs deletes batches that have passed their expiration on Ethereum
 // keep in mind several things when modifying this function
 // A) unlike nonces timeouts are not monotonically increasing, meaning batch 5 can have a later timeout than batch 6
-//    this means that we MUST only cleanup a single batch at a time
+// this means that we MUST only cleanup a single batch at a time
+//
 // B) it is possible for ethereumHeight to be zero if no events have ever occurred, make sure your code accounts for this
 // C) When we compute the timeout we do our best to estimate the Ethereum block height at that very second. But what we work with
-//    here is the Ethereum block height at the time of the last Deposit or Withdraw to be observed. It's very important we do not
-//    project, if we do a slowdown on ethereum could cause a double spend. Instead timeouts will *only* occur after the timeout period
-//    AND any deposit or withdraw has occurred to update the Ethereum block height.
+//
+//	here is the Ethereum block height at the time of the last Deposit or Withdraw to be observed. It's very important we do not
+//	project, if we do a slowdown on ethereum could cause a double spend. Instead timeouts will *only* occur after the timeout period
+//	AND any deposit or withdraw has occurred to update the Ethereum block height.
 func cleanupTimedOutBatchTxs(ctx sdk.Context, k keeper.Keeper) {
 	ethereumHeight := k.GetLastObservedEthereumBlockHeight(ctx).EthereumHeight
 	k.IterateOutgoingTxsByType(ctx, types.BatchTxPrefixByte, func(key []byte, otx types.OutgoingTx) bool {
@@ -250,12 +252,15 @@ func cleanupTimedOutBatchTxs(ctx sdk.Context, k keeper.Keeper) {
 // cleanupTimedOutContractCallTxs deletes logic calls that have passed their expiration on Ethereum
 // keep in mind several things when modifying this function
 // A) unlike nonces timeouts are not monotonically increasing, meaning call 5 can have a later timeout than batch 6
-//    this means that we MUST only cleanup a single call at a time
+//
+//	this means that we MUST only cleanup a single call at a time
+//
 // B) it is possible for ethereumHeight to be zero if no events have ever occurred, make sure your code accounts for this
 // C) When we compute the timeout we do our best to estimate the Ethereum block height at that very second. But what we work with
-//    here is the Ethereum block height at the time of the last Deposit or Withdraw to be observed. It's very important we do not
-//    project, if we do a slowdown on ethereum could cause a double spend. Instead timeouts will *only* occur after the timeout period
-//    AND any deposit or withdraw has occurred to update the Ethereum block height.
+//
+//	here is the Ethereum block height at the time of the last Deposit or Withdraw to be observed. It's very important we do not
+//	project, if we do a slowdown on ethereum could cause a double spend. Instead timeouts will *only* occur after the timeout period
+//	AND any deposit or withdraw has occurred to update the Ethereum block height.
 func cleanupTimedOutContractCallTxs(ctx sdk.Context, k keeper.Keeper) {
 	ethereumHeight := k.GetLastObservedEthereumBlockHeight(ctx).EthereumHeight
 	k.IterateOutgoingTxsByType(ctx, types.ContractCallTxPrefixByte, func(_ []byte, otx types.OutgoingTx) bool {

module/x/gravity/abci_test.go

@@ -234,7 +234,7 @@ func TestSignerSetTxSetting(t *testing.T) {
 	require.EqualValues(t, 1, len(gk.GetSignerSetTxs(ctx)))
 }
 
-/// Test batch timeout
+// Test batch timeout
 func TestBatchTxTimeout(t *testing.T) {
 	input, ctx := keeper.SetupFiveValChain(t)
 	gravityKeeper := input.GravityKeeper
@@ -263,12 +263,12 @@ func TestBatchTxTimeout(t *testing.T) {
 	ctx = ctx.WithBlockTime(now).WithBlockHeight(250)
 
 	// check that we can make a batch without first setting an ethereum block height
-	b1 := gravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	b1 := gravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 	require.Equal(t, b1.Timeout, uint64(0))
 
 	gravityKeeper.SetLastObservedEthereumBlockHeight(ctx, 500)
 
-	b2 := gravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	b2 := gravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 	// this is exactly block 500 plus twelve hours
 	require.Equal(t, b2.Timeout, uint64(504))
 
@@ -281,7 +281,7 @@ func TestBatchTxTimeout(t *testing.T) {
 	// when, way into the future
 	ctx = ctx.WithBlockTime(now).WithBlockHeight(9)
 
-	b3 := gravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	b3 := gravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 
 	gravity.BeginBlocker(ctx, gravityKeeper)
 

module/x/gravity/client/cli/tx.go

@@ -30,7 +30,6 @@ func GetTxCmd(storeKey string) *cobra.Command {
 	gravityTxCmd.AddCommand(
 		CmdSendToEthereum(),
 		CmdCancelSendToEthereum(),
-		CmdRequestBatchTx(),
 		CmdSetDelegateKeys(),
 	)
 
@@ -116,35 +115,6 @@ func CmdCancelSendToEthereum() *cobra.Command {
 	return cmd
 }
 
-func CmdRequestBatchTx() *cobra.Command {
-	cmd := &cobra.Command{
-		Use:   "request-batch-tx [denom] [signer]",
-		Args:  cobra.ExactArgs(2),
-		Short: "Request batch transaction for denom by signer",
-		RunE: func(cmd *cobra.Command, args []string) error {
-			clientCtx, err := client.GetClientTxContext(cmd)
-			if err != nil {
-				return err
-			}
-
-			denom := args[0]
-			signer, err := sdk.AccAddressFromHex(args[1])
-			if err != nil {
-				return err
-			}
-
-			msg := types.NewMsgRequestBatchTx(denom, signer)
-			if err = msg.ValidateBasic(); err != nil {
-				return err
-			}
-			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
-		},
-	}
-
-	flags.AddTxFlagsToCmd(cmd)
-	return cmd
-}
-
 func CmdSetDelegateKeys() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "set-delegate-keys [validator-address] [orchestrator-address] [ethereum-address] [ethereum-signature]",

module/x/gravity/handler.go

@@ -25,10 +25,6 @@ func NewHandler(k keeper.Keeper) sdk.Handler {
 			res, err := msgServer.CancelSendToEthereum(sdk.WrapSDKContext(ctx), msg)
 			return sdk.WrapServiceResult(ctx, res, err)
 
-		case *types.MsgRequestBatchTx:
-			res, err := msgServer.RequestBatchTx(sdk.WrapSDKContext(ctx), msg)
-			return sdk.WrapServiceResult(ctx, res, err)
-
 		case *types.MsgSubmitEthereumTxConfirmation:
 			res, err := msgServer.SubmitEthereumTxConfirmation(sdk.WrapSDKContext(ctx), msg)
 			return sdk.WrapServiceResult(ctx, res, err)

module/x/gravity/keeper/batch.go

@@ -14,14 +14,14 @@ import (
 // TODO: should we make this a parameter or a a call arg?
 const BatchTxSize = 100
 
-// BuildBatchTx starts the following process chain:
-// - find bridged denominator for given voucher type
-// - determine if a an unexecuted batch is already waiting for this token type, if so confirm the new batch would
-//   have a higher total fees. If not exit withtout creating a batch
-// - select available transactions from the outgoing transaction pool sorted by fee desc
-// - persist an outgoing batch object with an incrementing ID = nonce
-// - emit an event
-func (k Keeper) BuildBatchTx(ctx sdk.Context, contractAddress common.Address, maxElements int) *types.BatchTx {
+// CreateBatchTx starts the following process chain:
+//   - find bridged denominator for given voucher type
+//   - determine if a an unexecuted batch is already waiting for this token type, if so confirm the new batch would
+//     have a higher total fees. If not exit withtout creating a batch
+//   - select available transactions from the unbatched SendToEthereums sorted by fee desc
+//   - persist an OutgoingTx (BatchTx) object with an incrementing ID = nonce
+//   - emit an event
+func (k Keeper) CreateBatchTx(ctx sdk.Context, contractAddress common.Address, maxElements int) *types.BatchTx {
 	// if there is a more profitable batch for this token type do not create a new batch
 	if lastBatch := k.getLastOutgoingBatchByTokenType(ctx, contractAddress); lastBatch != nil {
 		if lastBatch.GetFees().GTE(k.getBatchFeesByTokenType(ctx, contractAddress, maxElements)) {

module/x/gravity/keeper/batch_test.go

@@ -41,7 +41,7 @@ func TestBatches(t *testing.T) {
 	ctx = ctx.WithBlockTime(now)
 
 	// tx batch size is 2, so that some of them stay behind
-	firstBatch := input.GravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	firstBatch := input.GravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 
 	// then batch is persisted
 	gotFirstBatch := input.GravityKeeper.GetOutgoingTx(ctx, firstBatch.GetStoreIndex())
@@ -81,7 +81,7 @@ func TestBatches(t *testing.T) {
 	// create the more profitable batch
 	ctx = ctx.WithBlockTime(now)
 	// tx batch size is 2, so that some of them stay behind
-	secondBatch := input.GravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	secondBatch := input.GravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 
 	// check that the more profitable batch has the right txs in it
 	expSecondBatch := &types.BatchTx{
@@ -160,7 +160,7 @@ func TestBatchesFullCoins(t *testing.T) {
 	ctx = ctx.WithBlockTime(now)
 
 	// tx batch size is 2, so that some of them stay behind
-	firstBatch := input.GravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	firstBatch := input.GravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 
 	// then batch is persisted
 	gotFirstBatch := input.GravityKeeper.GetOutgoingTx(ctx, firstBatch.GetStoreIndex())
@@ -228,7 +228,7 @@ func TestBatchesFullCoins(t *testing.T) {
 	// create the more profitable batch
 	ctx = ctx.WithBlockTime(now)
 	// tx batch size is 2, so that some of them stay behind
-	secondBatch := input.GravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	secondBatch := input.GravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 
 	// check that the more profitable batch has the right txs in it
 	expSecondBatch := &types.BatchTx{
@@ -340,7 +340,7 @@ func TestPoolTxRefund(t *testing.T) {
 	ctx = ctx.WithBlockTime(now)
 
 	// tx batch size is 2, so that some of them stay behind
-	input.GravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	input.GravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 
 	// try to refund a tx that's in a batch
 	err := input.GravityKeeper.cancelSendToEthereum(ctx, 2, mySender.String())
@@ -376,7 +376,7 @@ func TestEmptyBatch(t *testing.T) {
 
 	// no transactions should be included in this batch
 	ctx = ctx.WithBlockTime(now)
-	batchTx := input.GravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	batchTx := input.GravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 
 	require.Nil(t, batchTx)
 }

module/x/gravity/keeper/keeper_test.go

@@ -583,7 +583,7 @@ func TestKeeper_Migration(t *testing.T) {
 	ctx = ctx.WithBlockTime(now)
 
 	// tx batch size is 2, so that some of them stay behind
-	firstBatch := input.GravityKeeper.BuildBatchTx(ctx, myTokenContractAddr, 2)
+	firstBatch := input.GravityKeeper.CreateBatchTx(ctx, myTokenContractAddr, 2)
 
 	// then batch is persisted
 	gotFirstBatch := input.GravityKeeper.GetOutgoingTx(ctx, firstBatch.GetStoreIndex())

module/x/gravity/keeper/msg_server.go

@@ -239,35 +239,6 @@ func (k msgServer) SendToEthereum(c context.Context, msg *types.MsgSendToEthereu
 	return &types.MsgSendToEthereumResponse{Id: txID}, nil
 }
 
-// RequestBatchTx handles MsgRequestBatchTx
-func (k msgServer) RequestBatchTx(c context.Context, msg *types.MsgRequestBatchTx) (*types.MsgRequestBatchTxResponse, error) {
-	// TODO: limit this to only orchestrators and validators?
-	ctx := sdk.UnwrapSDKContext(c)
-
-	// Check if the denom is a gravity coin, if not, check if there is a deployed ERC20 representing it.
-	// If not, error out. Normalizes the format of the input denom if it's a gravity denom.
-	_, tokenContract, err := k.DenomToERC20Lookup(ctx, types.NormalizeDenom(msg.Denom))
-	if err != nil {
-		return nil, err
-	}
-
-	batchID := k.BuildBatchTx(ctx, tokenContract, BatchTxSize)
-	if batchID == nil {
-		return nil, fmt.Errorf("no suitable batch to create")
-	}
-
-	ctx.EventManager().EmitEvent(
-		sdk.NewEvent(
-			sdk.EventTypeMessage,
-			sdk.NewAttribute(sdk.AttributeKeyModule, msg.Type()),
-			sdk.NewAttribute(types.AttributeKeyContract, tokenContract.Hex()),
-			sdk.NewAttribute(types.AttributeKeyBatchNonce, fmt.Sprint(batchID.BatchNonce)),
-		),
-	)
-
-	return &types.MsgRequestBatchTxResponse{}, nil
-}
-
 func (k msgServer) CancelSendToEthereum(c context.Context, msg *types.MsgCancelSendToEthereum) (*types.MsgCancelSendToEthereumResponse, error) {
 	ctx := sdk.UnwrapSDKContext(c)