Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify mock contract keeper processCallback logic #4309

Closed
3 tasks
colin-axner opened this issue Aug 8, 2023 · 2 comments · Fixed by #4375
Closed
3 tasks

Simplify mock contract keeper processCallback logic #4309

colin-axner opened this issue Aug 8, 2023 · 2 comments · Fixed by #4375
Labels
callbacks middleware Issues for callbacks middleware testing Testing package and unit/integration tests

Comments

@colin-axner
Copy link
Contributor

Summary

The current function uses the inputted gas amount for a transaction to perform actions for testing:

// processMockCallback returns nil if the gas meter has greater than or equal to 500000 gas remaining.
// This function consumes 500000 gas, or the remaining gas if less than 500000.
// This function oog panics if the gas remaining is less than 400000.
func (k ContractKeeper) processMockCallback(
	ctx sdk.Context,
	callbackType callbacktypes.CallbackType,
	authAddress string,
) error {
	gasRemaining := ctx.GasMeter().GasRemaining()

	// increment stateful entries, if the callbacks module handler
	// reverts state, we can check by querying for the counter
	// currently stored.
	k.IncrementStateEntryCounter(ctx)

	// increment callback execution attempts
	k.Counters[callbackType]++

	if gasRemaining < 400000 {
		// consume gas will panic since we attempt to consume 500_000 gas, for tests
		ctx.GasMeter().ConsumeGas(500000, fmt.Sprintf("mock %s callback panic", callbackType))
	} else if gasRemaining < 500000 {
		ctx.GasMeter().ConsumeGas(gasRemaining, fmt.Sprintf("mock %s callback failure", callbackType))
		return MockApplicationCallbackError
	}

	if authAddress == MockCallbackUnauthorizedAddress {
		ctx.GasMeter().ConsumeGas(500000, fmt.Sprintf("mock %s callback unauthorized", callbackType))
		return MockApplicationCallbackError
	}

	ctx.GasMeter().ConsumeGas(500000, fmt.Sprintf("mock %s callback success", callbackType))
	return nil
}

This can be hard to maintain since changing the gas for a transaction will change the expected output.

We could alternatively use the callback address to decide what the mock callback should do:

// processMockCallback returns nil if the gas meter has greater than or equal to 500000 gas remaining.
// This function consumes 500000 gas, or the remaining gas if less than 500000.
// This function oog panics if the gas remaining is less than 400000.
func (k ContractKeeper) processMockCallback(
    ctx sdk.Context,
    callbackType callbacktypes.CallbackType,
    contractAddress string,
) error {
    gasRemaining := ctx.GasMeter().GasRemaining()

    // increment stateful entries, if the callbacks module handler
    // reverts state, we can check by querying for the counter
    // currently stored.
    k.IncrementStateEntryCounter(ctx)

    // increment callback execution attempts
    k.Counters[callbackType]++

    // consume a single gas to indicate gas consumption occurred
    ctx.GasMeter().ConsumeGas(1, fmt.Sprintf("mock %s callback panic", callbackType))

    switch contractAddress {
    case PanicContract:
        ctx.GasMeter().ConsumeGas(gasRemaining+1, fmt.Sprintf("mock %s callback panic", callbackType))
    case UnauthorizedContract, FailureContract:
        return MockApplicationCallbackError
    case SuccessContract:
        return nil 
    }   

    return nil
}

with

const (
    UnauthorizedContract = "unauthorized"
    PanicContract        = "panics out of gas"
    FailureContract      = "failure"
    SuccessContract      = "success"
)   

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added needs discussion Issues that need discussion before they can be worked on callbacks middleware Issues for callbacks middleware labels Aug 8, 2023
@colin-axner
Copy link
Contributor Author

If we do this, we can add test cases in the integration for using a FailureContract, I've removed the cases which tried executing a failure via a certain gas amount. It was too difficult to assert the contract failed in those cases

@colin-axner
Copy link
Contributor Author

When this issue is tackled, we should add failure contract test cases for all the table tests in ibc_middleware_test.go (a couple tests can be modified to use the new failure contract, while others will need to have the test case added)

@colin-axner colin-axner removed the needs discussion Issues that need discussion before they can be worked on label Aug 10, 2023
@crodriguezvega crodriguezvega added this to the callbacks/v1.0.0 milestone Aug 14, 2023
@srdtrk srdtrk added the testing Testing package and unit/integration tests label Aug 17, 2023
@crodriguezvega crodriguezvega moved this to Todo in ibc-go Aug 21, 2023
@crodriguezvega crodriguezvega moved this from Todo to In review in ibc-go Aug 21, 2023
@github-project-automation github-project-automation bot moved this from In review to Done in ibc-go Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
callbacks middleware Issues for callbacks middleware testing Testing package and unit/integration tests
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants