fix: Uniswap sim state propagation, Sentry user-workflow noise

[chrisli30]

• fix: silence expected user-workflow failures from Sentry error alerts (fix: silence expected user-workflow failures from Sentry error alerts #526)

Co-authored-by: Will Zimmerman will@avaprotocol.org

• fix: ERC20 approve state propagation for Uniswap workflow simulation (fix: ERC20 approve state propagation for Uniswap workflow simulation #527)

Co-authored-by: Will Zimmerman will@avaprotocol.org

[Copilot]

Pull request overview

This PR addresses two related operational issues in the workflow engine: (1) reducing Sentry alert noise by downgrading expected user-workflow failures from Error to Warn, and (2) fixing Uniswap simulation state propagation by explicitly injecting ERC20 allowance overrides after successful approve() simulations when Tenderly returns raw_state_diff: null.

Changes:

• Add preset.LogBundlerError / IsUserOpRevert and update bundler/UserOp call sites to log expected on-chain reverts at Warn (keeping infra/AA failures at Error).
• Inject ERC20 allowance storage overrides into SimulationStateMap after successful approve() simulation to unblock downstream swap simulations.
• Add a full-stop-loss Uniswap workflow Sepolia integration test and a changelog entry documenting the root cause and fix.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/erc4337/preset/bundler_error.go	Adds bundler error classification and severity routing (Warn for on-chain revert marker, Error otherwise).
pkg/erc4337/preset/bundler_error_test.go	Adds tests for the classification + logging dispatch behavior.
core/taskengine/vm_runner_contract_write.go	Merges sim diffs and injects allowance overrides after approve; routes bundler failures through LogBundlerError; reduces noisy “CRITICAL DEBUG” logs.
core/taskengine/simulation_state.go	Adds erc20AllowanceSlot and common allowance slot candidates for state override injection.
core/taskengine/simulate_uniswap_workflow_test.go	Adds an end-to-end Sepolia workflow simulation test covering approve→swap propagation.
core/taskengine/tenderly_client.go	Downgrades expected simulation reverts from Error to Warn.
core/taskengine/executor.go	Downgrades “completed with failures” task summary from Error to Warn.
core/taskengine/engine.go	Downgrades simulation “completed with failures” summary from Error to Warn.
core/taskengine/vm_runner_eth_transfer.go	Routes ETH transfer bundler failures through preset.LogBundlerError.
aggregator/rpc_server.go	Routes withdrawal UserOp failures through preset.LogBundlerError.
docs/changes/0002-uniswap-simulation-state-propagation.md	Documents the Tenderly raw_state_diff: null behavior and the allowance injection workaround.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

bundlerErrorSpyLogger does not appear to implement the full sdklogging.Logger interface (e.g., missing WithComponent/WithName/WithServiceName/WithHostName/Sync methods that other logger mocks in the repo implement). This will prevent the test from compiling. Consider embedding an existing no-op logger implementation and overriding only Warn/Error to record calls, or add the missing methods to the spy type.

Suggested change

	func (s *bundlerErrorSpyLogger) Debug(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Debugf(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Info(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Infof(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Warn(string, ...any) { s.record("Warn") }
	func (s *bundlerErrorSpyLogger) Warnf(string, ...any) { s.record("Warn") }
	func (s *bundlerErrorSpyLogger) Error(string, ...any) { s.record("Error") }
	func (s *bundlerErrorSpyLogger) Errorf(string, ...any) { s.record("Error") }
	func (s *bundlerErrorSpyLogger) Fatal(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Fatalf(string, ...any) {}
	func (s *bundlerErrorSpyLogger) With(...any) sdklogging.Logger { return s }
	func (s *bundlerErrorSpyLogger) Debug(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Debugf(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Info(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Infof(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Warn(string, ...any) { s.record("Warn") }
	func (s *bundlerErrorSpyLogger) Warnf(string, ...any) { s.record("Warn") }
	func (s *bundlerErrorSpyLogger) Error(string, ...any) { s.record("Error") }
	func (s *bundlerErrorSpyLogger) Errorf(string, ...any) { s.record("Error") }
	func (s *bundlerErrorSpyLogger) Fatal(string, ...any) {}
	func (s *bundlerErrorSpyLogger) Fatalf(string, ...any) {}
	func (s *bundlerErrorSpyLogger) With(...any) sdklogging.Logger { return s }
	func (s *bundlerErrorSpyLogger) WithComponent(string) sdklogging.Logger { return s }
	func (s *bundlerErrorSpyLogger) WithName(string) sdklogging.Logger { return s }
	func (s *bundlerErrorSpyLogger) WithServiceName(string) sdklogging.Logger { return s }
	func (s *bundlerErrorSpyLogger) WithHostName(string) sdklogging.Logger { return s }
	func (s *bundlerErrorSpyLogger) Sync() error { return nil }

[Copilot]

This Sepolia integration test hard-fails if the derived smart wallet has <2 USDC. Since this is an environment-dependent precondition (like other Sepolia tests that skip when unfunded), prefer t.Skip with a clear message when the balance is insufficient to avoid flaky/CI failures when OWNER_EOA is set but the wallet isn’t funded.

Suggested change

	require.True(t, usdcBalance.Cmp(big.NewInt(2_000_000)) >= 0,
	"wallet must hold at least 2 USDC; fund %s", smartWalletAddr.Hex())
	if usdcBalance.Cmp(big.NewInt(2_000_000)) < 0 {
	t.Skipf("skipping Sepolia integration test: smart wallet %s must hold at least 2 USDC", smartWalletAddr.Hex())
	}

[Copilot]

Allowance override parsing uses big.Int.SetString(..., 0), which treats strings with a leading 0 as octal (e.g., "010" → 8). To avoid incorrect allowance injection, consider parsing as base-10 unless the value has a 0x prefix, and also validate resolvedMethodParams[0] with common.IsHexAddress before HexToAddress to avoid silently injecting allowance for the zero address.

Suggested change

	spender := common.HexToAddress(resolvedMethodParams[0])
	// Base 0 auto-detects decimal, hex (0x...), and octal.
	amount, ok := new(big.Int).SetString(resolvedMethodParams[1], 0)
	if !ok || amount.Sign() < 0 {
	r.vm.logger.Debug("Skipping allowance override: invalid or negative amount",
	"raw", resolvedMethodParams[1])
	} else {
	valueHex := fmt.Sprintf("0x%064x", amount)
	for _, candidateSlot := range commonAllowanceSlots {
	slotHash := erc20AllowanceSlot(senderAddress, spender, candidateSlot)
	r.vm.simulationState.SetStorageSlot(contractAddress.Hex(), slotHash.Hex(), valueHex)
	}
	r.vm.logger.Debug("Injected allowance override after approve simulation",
	"token", contractAddress.Hex(),
	"owner", senderAddress.Hex(),
	"spender", spender.Hex(),
	"amount", amount.String(),
	"slots_set", len(commonAllowanceSlots))
	rawSpender := strings.TrimSpace(resolvedMethodParams[0])
	if !common.IsHexAddress(rawSpender) {
	r.vm.logger.Debug("Skipping allowance override: invalid spender address",
	"raw", resolvedMethodParams[0])
	} else {
	spender := common.HexToAddress(rawSpender)
	rawAmount := strings.TrimSpace(resolvedMethodParams[1])
	base := 10
	if strings.HasPrefix(rawAmount, "0x") || strings.HasPrefix(rawAmount, "0X") {
	base = 16
	}
	amount, ok := new(big.Int).SetString(rawAmount, base)
	if !ok || amount.Sign() < 0 {
	r.vm.logger.Debug("Skipping allowance override: invalid or negative amount",
	"raw", resolvedMethodParams[1])
	} else {
	valueHex := fmt.Sprintf("0x%064x", amount)
	for _, candidateSlot := range commonAllowanceSlots {
	slotHash := erc20AllowanceSlot(senderAddress, spender, candidateSlot)
	r.vm.simulationState.SetStorageSlot(contractAddress.Hex(), slotHash.Hex(), valueHex)
	}
	r.vm.logger.Debug("Injected allowance override after approve simulation",
	"token", contractAddress.Hex(),
	"owner", senderAddress.Hex(),
	"spender", spender.Hex(),
	"amount", amount.String(),
	"slots_set", len(commonAllowanceSlots))
	}

[claude]

Code Review

Overview

This PR bundles two well-scoped production fixes:

1. Sentry noise (fix: silence expected user-workflow failures from Sentry error alerts #526): Downgrades expected user-workflow failures from Error to Warn so on-chain reverts and simulation failures stop triggering Sentry alerts; real infra/AA failures stay at Error.
2. Uniswap approve state propagation (fix: ERC20 approve state propagation for Uniswap workflow simulation #527): Explicitly injects allowance[owner][spender] storage slots after a successful approve() simulation, working around Tenderly returning raw_state_diff: null for that call.

Both fixes are well-motivated, and the PR includes the integration test used to diagnose and verify the Uniswap issue. The docs/changes/ entry is a useful root-cause record.

---

What's good

• Debug log cleanup: Removes the BUNDLER FAILED and CRITICAL DEBUG log lines with emojis that should never have been in production paths.
• preset.LogBundlerError abstraction: Centralizes Warn/Error dispatch rather than duplicating the classification at each call site. The nil-logger guard is a nice safety net.
• erc20AllowanceSlot formula: Correctly implements Solidity's nested mapping layout (keccak256(spender || keccak256(owner || slot))).
• Test coverage: Unit tests for bundler_error.go using a spy logger; integration test with proper skip guards (testing.Short(), OWNER_EOA env).
• Comment quality: Comments explain WHY (Tenderly limitation, Sentry rationale), not just what the code does.

---

Issues

Moderate

1. IsUserOpRevert is tightly coupled to the builder.go error strings

pkg/erc4337/preset/bundler_error.go

The marker string "success=false in UserOperationEvent" is hard-coded in bundler_error.go and matched by substring. It is produced by three separate fmt.Errorf calls in builder.go (lines 205, 227, 274). If any of those messages is edited, the classification silently regresses -- infra errors get downgraded to Warn, or on-chain reverts start paging Sentry.

The existing tests use hard-coded strings, not the actual error path:

errors.New("UserOp execution failed (success=false in UserOperationEvent) - tx: 0xabc")

Consider exporting the marker constant from builder.go and importing it in bundler_error.go, linking the two at compile time:

// builder.go
const UserOpRevertMarker = "success=false in UserOperationEvent"
// bundler_error.go uses preset.UserOpRevertMarker (or builder.UserOpRevertMarker)

2. Allowance slot injection does not probe -- balance injection does

core/taskengine/simulation_state.go, vm_runner_contract_write.go

ProbeERC20BalanceSlot discovers which storage slot a specific token uses before injecting by comparing eth_getStorageAt against balanceOf. For allowances, all four candidate slots are blindly set for every approve() call. A token at slot 2 gets allowance values written at slots 1, 3, and 10 as well -- spurious state that could interact unexpectedly with later simulation steps that also read allowances.

The docs acknowledge this as a known limitation and the impact is simulation-only. Worth flagging as a TODO for a ProbeERC20AllowanceSlot equivalent.

---

Minor

3. fetchERC20Balance silently returns 0 on RPC error

core/taskengine/simulate_uniswap_workflow_test.go:500

When the RPC call fails, the function returns 0, causing the downstream require.True(usdcBalance.Cmp(big.NewInt(2_000_000)) >= 0) to fail with a misleading "insufficient balance" message. t.Fatalf here would make the root cause immediately obvious.

4. Magic bytes in fetchERC20Balance calldata

core/taskengine/simulate_uniswap_workflow_test.go:492

[]byte{0x70, 0xa0, 0x82, 0x31} is the 4-byte selector for keccak256("balanceOf(address)"). A one-line comment would make this legible without a lookup.

5. isMethodWithParams uses case-insensitive match for ABI method names

core/taskengine/vm_runner_contract_write.go:78

EVM ABI method names are case-sensitive -- approve and Approve produce different 4-byte selectors. For standard ERC-20 the lowercase form is universal, but a comment acknowledging this assumption prevents confusion if this helper is reused for other methods.

6. sv helper name is overly abbreviated

core/taskengine/simulate_uniswap_workflow_test.go:481

sv is used ~15 times in the test but the name does not hint at its purpose. mustStructValue or toStructPb would be readable without prior context.

7. No validation of resolvedMethodParams[0] before common.HexToAddress

core/taskengine/vm_runner_contract_write.go:508

HexToAddress silently returns the zero address for any non-hex input. If a template variable fails to resolve, the injected slot targets the wrong address with no log entry. A basic check (len == 42 && strings.HasPrefix(_, "0x")) with a Debug log on mismatch would make the failure explicit.

---

Summary

The fixes are correct and well-tested. The main item worth addressing before merge is the coupling between userOpRevertMarker and the three builder.go error strings -- a compile-time link rather than a duplicated string would prevent silent regressions. The rest are minor quality improvements.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This integration test hard-fails when OWNER_EOA is not set (require.True). Other Sepolia integration tests in this package typically t.Skip when required env/config isn’t present, so this will likely break CI/local runs. Prefer skipping when OWNER_EOA is missing (and consider adding an //go:build integration tag to keep this external-RPC test out of default go test ./...).

Suggested change

	require.True(t, ok, "OWNER_EOA must be set to run this Sepolia integration test")
	if !ok {
	t.Skip("Skipping Sepolia integration test: OWNER_EOA is not set")
	}