fix: ensure module param changes dont mess up callbacks (#537)

* storing the max gas value when registering the callback to ensure reducing the param does not err the callback

* updating tests

* linting

* Update CHANGELOG.md

---------

Signed-off-by: Spoorthi <[email protected]>

CHANGELOG.md

@@ -63,7 +63,8 @@ Contains all the PRs that improved the code without changing the behaviors.
 - [#476](https://github.com/archway-network/archway/pull/476) - Fix amd64 binary compatibility on newer linux OS
 - [#514](https://github.com/archway-network/archway/pull/514) - Fix snapshot db being hardcoded from goleveldb to based on config 
 - [#522](https://github.com/archway-network/archway/pull/522) - Fix Archway module endpoints not showing up in swagger
-- [#538](https://github.com/archway-network/archway/pull/538) - Fix Archway module endpoints not showing up in swagger
+- [#537](https://github.com/archway-network/archway/pull/537) - Fix issue with callback failing when module param is changed
+- [#538](https://github.com/archway-network/archway/pull/538) - Fixing the interchain test gh workflow failing cuz rc tags were not recognized 
 
 ### Improvements
 

proto/archway/callback/v1/callback.proto

@@ -20,6 +20,8 @@ message Callback {
     CallbackFeesFeeSplit fee_split = 4;
     // reserved_by is the address which reserved the callback (bech32 encoded).
     string reserved_by = 5;
+    // callback_gas_limit is the maximum gas that can be consumed by this callback.
+    uint64 max_gas_limit = 6;
 }
 
 // CallbackFeesFeeSplit is the breakdown of all the fees that need to be paid by the contract to reserve a callback

x/callback/abci.go

@@ -12,18 +12,13 @@ import (
 
 // EndBlocker fetches all the callbacks registered for the current block height and executes them
 func EndBlocker(ctx sdk.Context, k keeper.Keeper, wk types.WasmKeeperExpected) []abci.ValidatorUpdate {
-	params, err := k.Params.Get(ctx)
-	if err != nil {
-		panic(err)
-	}
-
 	currentHeight := ctx.BlockHeight()
-	k.IterateCallbacksByHeight(ctx, currentHeight, callbackExec(ctx, k, wk, params.GetCallbackGasLimit()))
+	k.IterateCallbacksByHeight(ctx, currentHeight, callbackExec(ctx, k, wk))
 	return nil
 }
 
 // callbackExec returns a function which executes the callback and deletes it from state after execution
-func callbackExec(ctx sdk.Context, k keeper.Keeper, wk types.WasmKeeperExpected, callbackGasLimit uint64) func(types.Callback) bool {
+func callbackExec(ctx sdk.Context, k keeper.Keeper, wk types.WasmKeeperExpected) func(types.Callback) bool {
 	logger := k.Logger(ctx)
 	return func(callback types.Callback) bool {
 		// creating CallbackMsg which is encoded to json and passed as input to contract execution
@@ -36,7 +31,7 @@ func callbackExec(ctx sdk.Context, k keeper.Keeper, wk types.WasmKeeperExpected,
 			"msg", callbackMsg.String(),
 		)
 
-		gasUsed, err := pkg.ExecuteWithGasLimit(ctx, callbackGasLimit, func(ctx sdk.Context) error {
+		gasUsed, err := pkg.ExecuteWithGasLimit(ctx, callback.MaxGasLimit, func(ctx sdk.Context) error {
 			// executing the callback on the contract
 			_, err := wk.Sudo(ctx, sdk.MustAccAddressFromBech32(callback.ContractAddress), callbackMsg.Bytes())
 			return err
@@ -59,11 +54,11 @@ func callbackExec(ctx sdk.Context, k keeper.Keeper, wk types.WasmKeeperExpected,
 			)
 
 			// This is because gasUsed amount returned is greater than the gas limit. cuz ofc.
-			// so we set it to callbackGasLimit so when we do txFee refund, we arent trying to refund more than we should
-			// e.g if callbackGasLimit is 10, but gasUsed is 100, we need to use 10 to calculate txFeeRefund.
+			// so we set it to callback.MaxGasLimit so when we do txFee refund, we arent trying to refund more than we should
+			// e.g if callback.MaxGasLimit is 10, but gasUsed is 100, we need to use 10 to calculate txFeeRefund.
 			// else the module will pay back more than it took from the user 💀
 			// TLDR; this ensures in case of "out of gas error", we keep all txFees and refund nothing.
-			gasUsed = callbackGasLimit
+			gasUsed = callback.MaxGasLimit
 		} else {
 			logger.Info(
 				"callback executed successfully",

x/callback/abci_test.go

@@ -95,21 +95,40 @@ func TestEndBlocker(t *testing.T) {
 		})
 	}
 
-	// To test when callback exceeds gas limit. Setting module params callbackGasLimit to 1.
 	params, err := keeper.GetParams(ctx)
 	require.NoError(t, err)
-	err = keeper.SetParams(ctx, types.Params{
-		CallbackGasLimit:               1,
-		MaxBlockReservationLimit:       params.MaxBlockReservationLimit,
-		MaxFutureReservationLimit:      params.MaxFutureReservationLimit,
-		FutureReservationFeeMultiplier: params.FutureReservationFeeMultiplier,
-		BlockReservationFeeMultiplier:  params.BlockReservationFeeMultiplier,
-	})
+
+	// TEST CASE: Test CallbackGasLimit limit value reduced
+	// First we set the params value to default
+	// Register a callback for next block
+	reqMsg := &types.MsgRequestCallback{
+		ContractAddress: contractAddr.String(),
+		JobId:           INCREMENT_JOBID,
+		CallbackHeight:  ctx.BlockHeight() + 1,
+		Sender:          contractAdminAcc.Address.String(),
+		Fees:            feesToPay,
+	}
+	_, err = msgServer.RequestCallback(sdk.WrapSDKContext(ctx), reqMsg)
 	require.NoError(t, err)
 
+	// Setting the callbackGasLimit param to 1
+	params.CallbackGasLimit = 1
+	err = keeper.SetParams(ctx, params)
+	require.NoError(t, err)
+
+	// Increment block height and run end blocker
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
+	_ = callbackabci.EndBlocker(ctx, keeper, chain.GetApp().Keepers.WASMKeeper)
+
+	// Checking if the count value has incremented.
+	// Should have incremented as the callback should have access to higher gas limit as it was registered before the gas limit was reduced
+	count := getCount(t, chain, ctx, contractAddr)
+	require.Equal(t, initMsg.Count+1, count)
+
+	// TEST CASE: OUT OF GAS ERROR
 	// Reserving a callback for next block
 	// This callback should fail as it consumes more gas than allowed
-	reqMsg := &types.MsgRequestCallback{
+	reqMsg = &types.MsgRequestCallback{
 		ContractAddress: contractAddr.String(),
 		JobId:           INCREMENT_JOBID,
 		CallbackHeight:  ctx.BlockHeight() + 1,
@@ -124,8 +143,8 @@ func TestEndBlocker(t *testing.T) {
 	_ = callbackabci.EndBlocker(ctx, keeper, chain.GetApp().Keepers.WASMKeeper)
 
 	// Checking if the count value has incremented. Should not have incremented as the callback failed due to out of gas error
-	count := getCount(t, chain, ctx, contractAddr)
-	require.Equal(t, initMsg.Count, count)
+	count = getCount(t, chain, ctx, contractAddr)
+	require.Equal(t, initMsg.Count+1, count)
 }
 
 // getCount is a helper function to get the contract's count value

x/callback/keeper/callback.go

@@ -120,6 +120,16 @@ func (k Keeper) SaveCallback(ctx sdk.Context, callback types.Callback) error {
 		return types.ErrBlockFilled
 	}
 
+	// Setting the callback gas limit to the module param CallbackGasLimit.
+	// This is to ensure that if the param value is decreased in the future, before the callback is executed,
+	// it does not fail with "out of gas" error. it wouldnt be fair for the contract to err out of the callback
+	// if it wouldnt have been expected to at the time of registration
+	params, err = k.GetParams(ctx)
+	if err != nil {
+		return err
+	}
+	callback.MaxGasLimit = params.GetCallbackGasLimit()
+
 	return k.Callbacks.Set(ctx, collections.Join3(callback.GetCallbackHeight(), contractAddress.Bytes(), callback.GetJobId()), callback)
 }