KeeperHub / KeeperHub/keeperhub
feat(protocols): add Uniswap V3 collect-fees, decrease-liquidity and increase-liquidity actions
- Dominant language
- TypeScript
- Stars
- 24
- Forks
- 93
- Avg merge
- 1d 4h
- Merged PRs (30d)
- 253
Description
### Before filing
- [x] I searched open and closed issues for this proposal.
- [x] I checked the docs and the current behaviour on `staging`.
- [x] This is one change, not several. (Several means several issues.)
### Reason: what you cannot do today
Checked against `staging` at `fa1e05607`.
A Uniswap V3 LP cannot automate the three jobs LPs actually run: collecting earned fees, taking liquidity out to exit an out-of-range position or rebalance, and adding liquidity back to compound. `protocols/uniswap-v3.ts` wires the NonfungiblePositionManager on five chains but exposes only `get-position`, `balance-of`, `owner-of`, `approve-position`, `transfer-position` and `burn-position`, and `protocols/abis/uniswap-position-manager.json` carries only `positions`, `balanceOf`, `ownerOf`, `approve`, `transferFrom` and `burn`.
That also leaves `burn-position` unreachable in practice. `burn` requires zero liquidity and zero `tokensOwed`, and only `decreaseLiquidity` followed by `collect` produces that state - neither of which exists. At mainnet block 25976141, `burn(1)` called as the owner of position #1 reverts `Not cleared`. The protocol's own test data works around this by burning #100000, a position already emptied outside KeeperHub (`uniswap-v3.ts:15-19`, `:22`).
This came out of mapping which recurring keeper jobs the integrated protocols can perform. The Base position manager alone holds 4,647,847 positions (`totalSupply`), and none of their owners can collect a fee through KeeperHub.
### Reason: what the workaround costs
The only route today is a raw `web3/write-contract` node: paste the position manager ABI, hand-build a tuple argument, type 2^128 - 1 to mean "collect everything", and compute a unix deadline - with no labels, help text or validation, on a call that moves the caller's funds. An author who gets `amount0Max`, the recipient or the deadline wrong finds out on-chain.
There is also a trap no workaround surfaces. `get-position` returns `tokensOwed0` and `tokensOwed1`, but Uniswap only updates them when a position is touched, so they omit every fee earned since. At block 25976141 position #1 stores `tokensOwed0 = 0` and `tokensOwed1 = 0`, while a simulated `collect` as its owner returns `84250230863135893` and `661007116889360`. A workflow conditioned on `tokensOwed0 > 0` looks correct and never fires.
### Scope: what this touches, and what it does not
**Touches:** `protocols/uniswap-v3.ts` (three overrides on the existing `positionManager` contract, plus test data) and `protocols/abis/uniswap-position-manager.json` (three ABI entries). No new contract and no new address: the position manager is already configured for chains 1, 8453, 42161, 10 and 11155111.
**Does not touch:** the six existing position actions, swaps or quotes; the protocol registry; `protocol-write`. The new writes route through `plugins/protocol/steps/protocol-write.ts` like every protocol write, so they inherit its value cap and signer resolution as they stand today - including the `web3Connection` omission noted in #2431, which they neither worsen nor fix.
**One change?** There is a seam, and I would rather name it than tick past it. `collect-fees` and `decrease-liquidity` are coupled: `decreaseLiquidity` credits tokens to the position without transferring them, so without `collect` it strands the caller's funds, and together they are what makes `burn-position` reachable. `increase-liquidity` is not coupled and could ship alone. I have kept it here because compounding - collect, then add back - is the job most LPs automate, but if triage reads the seam the other way I will move it to a follow-up.
### Plan: what you propose
Three overrides on `positionManager`, in the struct-flattening idiom `swap-exact-input` already uses:
| Action | Function | Inputs |
|---|---|---|
| `collect-fees` | `collect(CollectParams)` | `tokenId`, `recipient`, `amount0Max`, `amount1Max` - both max fields default to `340282366920938463463374607431768211455` (uint128 max, "collect everything") |
| `decrease-liquidity` | `decreaseLiquidity(DecreaseLiquidityParams)` | `tokenId`, `liquidity`, `amount0Min`, `amount1Min`, `deadline` |
| `increase-liquidity` | `increaseLiquidity(IncreaseLiquidityParams)` | `tokenId`, `amount0Desired`, `amount1Desired`, `amount0Min`, `amount1Min`, `deadline` |
Outputs are the named returns: `amount0` and `amount1`, plus `liquidity` for `increase-liquidity`. `deadline` follows Aerodrome's existing `Deadline (unix timestamp)` label.
Help text states the behaviours an author cannot see from the ABI: `decrease-liquidity` does not transfer tokens until `collect-fees` runs; `get-position`'s `tokensOwed` fields exclude fees accrued since the position was last touched; and `increase-liquidity` needs an allowance on both tokens to the position manager, which the existing `web3/approve-token` action covers.
**Tests.** The fork harness already provisions owned positions through `forkImpersonatedCalls`. I would add one live position with liquidity - distinct from #1, so the `get-position` read expectation is untouched - and expect nonzero `amount0`/`amount1` from a partial `decrease-liquidity` followed by `collect-fees`.
**Existing callers:** unaffected. Purely additive.
**What I still need to determine:** whether `requiredTokens` can fund both tokens of a position's pair on the fork. That decides whether `increase-liquidity` is exercised in the harness or listed in `skipped` with its reason.
### Plan: alternatives you considered
- **Do nothing.** LPs keep a raw write-contract node or leave KeeperHub for the jobs they most want automated, and `burn-position` stays usable only on positions emptied elsewhere.
- **Document the raw `web3/write-contract` route instead.** Rejected: it moves user funds through a hand-encoded tuple with no labels or validation, and it still leaves the `tokensOwed` trap undocumented.
- **Include `mint` to open new positions.** Rejected for this change: an eleven-field struct whose ticks must align to the pool's tick spacing is a different class of risk and deserves its own issue.
- **A single atomic "exit position" action** (decrease, collect and burn via `multicall`). Rejected: the registry maps one action to one function, two or three nodes in a workflow already express the sequence, and each step stays individually inspectable.
### Scope: compatibility
- [ ] Changes an existing response shape, status code, CLI flag, or default.
- [ ] Adds, removes, or upgrades a dependency.
- [ ] Changes database schema or requires a migration.
- [x] Touches authentication, permissions, validation, or spend limits.
- [ ] Changes pricing, plan limits, or anything a user is charged.
Contributor guide
Research direction
Start in protocols/uniswap-v3.ts and protocols/abis/uniswap-position-manager.json, using the existing swap-exact-input override as the struct-flattening reference. Then inspect the fork harness and forkImpersonatedCalls setup, including requiredTokens, before running the relevant Uniswap V3 tests. Done means the three actions and ABI entries work with the documented inputs, outputs, and fee/liquidity behavior, with increase-liquidity tested or explicitly skipped with its reason.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- blockchain
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 62/100