evm: FindSpender fix

[neavra]

Summary by CodeRabbit

Release Notes

• Refactor
  • Restructured internal swap service to improve best quote selection and transaction building across supported providers.
  • Enhanced quote handling and dynamic spender management for EVM-based swap operations.
  • Improved error messaging and consistency in swap processing workflows.

[coderabbitai]

Walkthrough

Introduced a quote-based abstraction for EVM swap providers. Added QuoteResult type and Quote method to the provider interface. Renamed FindBestAmountOut to FindBestQuote, returning QuoteWinner with provider and quote data. Implemented the new method across multiple provider implementations with internal quote resolution flows.

Changes

Cohort / File(s)	Summary
EVM Provider Interface internal/evm/provider.go	Added QuoteResult type with AmountOut and Spender fields. Extended Provider interface with new Quote method signature.
EVM Swap Service internal/evm/swap_service.go	Added QuoteWinner type. Renamed FindBestAmountOut to FindBestQuote with new return signature. Replaced provider result aggregation to use QuoteResult and select best quote by amount.
Provider Implementations internal/mayachain/provider_evm.go, internal/oneinch/provider.go, internal/thorchain/provider_evm.go	Implemented public Quote method in each provider. Added internal quote resolution flows (resolveQuote, resolveSwap) with decimal conversion and amount normalization. Refactored MakeTx to use resolved quote data structures.
Consumer Integration internal/recurring/consumer.go	Updated EVM swap handling to use FindBestQuote flow. Removed legacy meta-rule resolution and findSpender helper. Simplified handleEvmSwap method signature. Dynamic spender determination based on winning quote provider.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'evm: FindSpender fix' is vague and does not accurately reflect the substantial refactoring performed. The PR introduces a comprehensive quote-based architecture across multiple files, including new Quote methods, QuoteResult/QuoteWinner types, and removal of legacy metarule logic—far beyond a simple FindSpender fix.	Provide a more descriptive title that reflects the main change, such as 'evm: Implement quote-based provider architecture' or 'evm: Refactor swap service to use Quote methods instead of FindSpender'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch 484-findspender-provider-mismatch

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/recurring/consumer.go (1)

1102-1105: ⚠️ Potential issue | 🟡 Minor

Pre-existing bug: %w wraps a stale/nil err instead of reporting the unparseable value.

SetString signals failure via ok == false, not an error. At this point err holds the result from a prior unrelated call (or is nil), so the formatted message will be misleading (e.g., "failed to parse fromAmountStr: <nil>").

🐛 Proposed fix

 	fromAmountTyped, ok := new(big.Int).SetString(fromAmount, 10)
 	if !ok {
-		return fmt.Errorf("failed to parse fromAmountStr: %w", err)
+		return fmt.Errorf("failed to parse fromAmount: %s", fromAmount)
 	}

🤖 Fix all issues with AI agents

In `@internal/evm/swap_service.go`:
- Around line 83-93: The code accesses result.quote.AmountOut without ensuring
result.quote is non-nil, which can panic if a provider returns (nil, nil);
update the logic in the function handling provider results to first check if
result.quote != nil before logging AmountOut or comparing AmountOut to
bestAmountOut, and skip or treat the result as invalid when result.quote is nil;
ensure you also avoid calling result.quote.AmountOut.Cmp when AmountOut is nil
by checking both result.quote != nil and result.quote.AmountOut != nil before
updating bestAmountOut, bestQuote, bestProvider, and bestProviderName, and
adjust the Debug log to safely include amountOut only when quote is present.

In `@internal/thorchain/provider_evm.go`:
- Around line 156-159: The THORChain provider currently calls strconv.ParseUint
on quote.DustThreshold unguarded, which will error if the string is empty;
mirror the Mayachain provider's behavior by checking quote.DustThreshold != ""
before parsing and defaulting dustThreshold to 0 when empty. Update the logic
around dustThreshold (and related variables) so that if quote.DustThreshold ==
"" you set dustThreshold = 0, otherwise parse with strconv.ParseUint and return
the existing wrapped error on parse failure.

🧹 Nitpick comments (3)

internal/oneinch/provider.go (1)

98-117: Double API call: Quote + MakeTx both hit the 1inch swap endpoint.

resolveSwap calls GetSwap (the actual swap resolution endpoint). When the consumer flow calls Quote during FindBestQuote and then MakeTx on the winner, 1inch's swap endpoint is invoked twice for the winning provider. This is functionally correct but worth noting — swap parameters (price, route) may shift between the two calls.

Additionally, consider whether the spender could be derived from rs.resp.Tx.To (the router address already returned in the swap response) instead of a separate GetSpender RPC, saving one API round-trip.

internal/mayachain/provider_evm.go (2)

228-288: MakeTx calls resolveQuote again, making a second API request to Mayachain.

In the consumer flow, Quote is called first (during FindBestQuote), and then MakeTx is called on the winner. Both invoke resolveQuote, which fetches a fresh quote from the Mayachain API. The second quote may return different amounts or parameters. Additionally, convertExpectedOut (line 282) may trigger an RPC call for token decimals a second time.

This is inherent to the two-phase design and functionally correct, but be aware that the amountOut returned by MakeTx can differ from what FindBestQuote selected.

---

74-93: Significant code duplication with internal/thorchain/provider_evm.go.

convertDecimals, convertExpectedOut, resolvedQuote, resolveQuote, getTokenDecimals, getTokenDecimalsWithRpc, and getTargetChainRpc are nearly identical between the Mayachain and Thorchain EVM providers. Consider extracting shared logic into a common helper package (e.g., internal/evm/dexchain) to reduce maintenance burden.

Also applies to: 178-205

[evgensheff]

LGTM