Skip to content

Implement Universal Router support and improve nonce management#61

Open
mitakash wants to merge 3 commits intomasterfrom
uniswap-UniversalRouter
Open

Implement Universal Router support and improve nonce management#61
mitakash wants to merge 3 commits intomasterfrom
uniswap-UniversalRouter

Conversation

@mitakash
Copy link
Collaborator

Changes

This PR adds support for swapping tokens using Uniswap's Universal Router protocol and significantly improves transaction nonce management.

Universal Router

  • Implemented new universal-router-module.ts with support for token swaps via Permit2
  • Added configuration types for Universal Router settings
  • Integrated with existing DexRouter system
  • Improved gas efficiency through better transaction routing

Nonce Management

  • Refactored all transaction functions to use a robust queueTransaction pattern
  • Added automatic nonce recovery when transactions fail
  • Centralized nonce tracking to prevent conflicts
  • Maintained backward compatibility for existing code

Testing

  • Tested on Avalanche with savUSD/USDC and custom tokens
  • Successfully executed multiple liquidations with proper token swaps
  • Verified error recovery when nonce conflicts occur
  • Observed reliable operation over extended periods

Future Work

  • Consider refactoring uniswap.ts to use the same nonce management pattern
  • Add additional Universal Router commands beyond basic swaps

@mitakash mitakash requested a review from EdNoepel May 11, 2025 18:24
Copy link
Owner

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few blocking issues:

  • Existing reward-action-tracker unit tests are broken by this change.
  • No test coverage for new universal-router-module.

Other concerns:
The keeper relies on ethers.js transaction management to handle nonces. There is a new queueTransaction facility which does not have test coverage. It is exclusively used in the new universal-router-module and not elsewhere in the keeper. And when it is used, the enqueue operation is awaited, making it a synchronous call, which seemingly defeats the purpose of having a queue.

We should define what problem this facility attempts to solve, implement tests to cover it, and adopt throughout the keeper. As such, please de-scope this change from the PR and create a new PR for it.


if (permit2Allowance.lt(amount)) {
logger.info(`Approving Permit2 to spend ${tokenToSwap.symbol}`);
await NonceTracker.queueTransaction(signer, async (nonce) => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're adding to a queue, why await the call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The await is definitely needed because queueTransaction isn't actually adding to an asynchronous processing queue - it's managing transaction nonces while executing the transaction.
Despite its name, queueTransaction is:

Getting the next proper nonce from the tracker
Immediately executing the transaction with that nonce
Handling failures by resetting the nonce when needed
Returning the transaction result

If we didn't await it, we would:

Never know if the transaction succeeded or failed
Not get the transaction result (receipt, etc.)
Create potential race conditions with nonce management
Have no way to properly handle errors

The "queue" in the name refers to how we're managing nonces to ensure proper transaction ordering, not that we're queuing work to be processed later. The function synchronously executes the transaction while handling the nonce management for us.

mitakash added 2 commits May 13, 2025 14:33
…d are test coverage for universal-router-module and we still need to fix reward-action-tracker broken unit tests, possibly split this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments