SIG-625: accept ERC721 approved operators for position lifecycle auth

[worjs]

Context

TradeModule lifecycle functions (increasePosition, decreasePosition, closePosition, claimPayout) required msg.sender to be the exact NFT owner. This forced SignalsRouter into a take→return custody pattern (transfer NFT to router, execute, transfer back), which caused all trade events to emit the router address as trader and generated spurious Transfer events. Subgraph, fee policy, and user stats were all attributed to the router contract instead of the actual user.

SIG-625

Decisions

• Added _resolvePositionOwner(positionId) in TradeModule that accepts ERC721 getApproved and isApprovedForAll operators, returning the actual NFT owner. All lifecycle functions now use the resolved owner for event emission and fee-policy trader attribution, while keeping token pulls/pushes on msg.sender.
• Removed the router's _takePosition/_returnPositionIfNeeded custody pattern entirely. Router lifecycle entrypoints now verify ownerOf(positionId) == msg.sender upfront and call Core directly as an approved operator.
• _decreasePositionInternal returns a 5th value (trader) so decreasePosition and closePosition can emit the resolved owner without a redundant ownerOf call.
• Added MockTraderFeePolicy test double that returns different fees based on params.trader to prove fee-policy attribution correctness.

Impact

Event semantics change for router-mediated lifecycle calls:

• PositionIncreased, PositionDecreased, PositionClosed, PositionClaimed, PositionSettled, TradeFeeCharged — trader field now contains the actual NFT owner instead of the router address.
• Two spurious Transfer events per router lifecycle call are eliminated.
• v1-subgraph: Trade event handlers already index on the trader field. No mapping changes needed — the correct address now arrives automatically. However, historical data indexed before this change will still have the router address.
• v1-sdk: No ABI signature changes. No SDK update needed.

No sibling PRs exist yet. Subgraph reindexing may be needed after deployment to correct historical router-attributed trades.

Risk Areas

• Authorization logic change in _resolvePositionOwner: now accepts getApproved and isApprovedForAll in addition to direct ownership — broadens the set of callers that can operate on a position.
• Sponsored position payout splitting with operator callers: operator receives user-profit portion, sponsor receives principal. The _splitSponsoredPayout logic pushes funds to msg.sender (operator) for the user portion.
• batchClaimPayout resolves ownership per-position inside the loop — each _processClaimPayout call runs a separate _resolvePositionOwner check.

[signals-reviewer]

Summary

This PR refactors position lifecycle authorization in TradeModule and SignalsRouter to accept ERC721 approved operators (getApproved and isApprovedForAll) in addition to direct NFT owners. The new _resolvePositionOwner(positionId) function resolves the actual NFT owner for event emission and fee-policy trader attribution, while keeping all fund pulls/pushes on msg.sender. The router's take-and-return custody pattern (_takePosition/_returnPositionIfNeeded) is removed entirely — the router now calls Core directly as an approved operator, eliminating spurious NFT Transfer events and fixing trader attribution in all trade events.

Cross-PR Context

Sibling PRs (SIG-625)

No sibling PRs found in other repositories.

Impact Map Check

• v1-subgraph: TradeModule.sol changes affect trade event handlers. However, no event signatures changed — same event names, same parameter types and count. The trader field now contains the actual NFT owner instead of the router address, which is a semantic improvement. Subgraph mappings continue to work without modification. No sibling PR required.
• v1-sdk: No ABI signature changes. The _resolvePositionOwner function is internal and does not affect the external ABI. No SDK update needed. No sibling PR required.
• Historical data: The PR description correctly notes that historical subgraph data indexed before deployment will still have the router address as trader. Reindexing may be needed post-deployment.

Issues

No issues found.

Suggestions

No suggestions.

Verdict

APPROVE: The authorization change is well-scoped and follows standard ERC721 approval semantics. Key verification points:

1. _resolvePositionOwner correctness: Checks ownerOf first (fast path for direct owner), then getApproved and isApprovedForAll — the complete ERC721 authorization surface. Returns the actual owner (not msg.sender), which is the correct value for event emission and fee-policy attribution.

2. Fund flow integrity: All _pullPayment and _pushPayment calls remain on msg.sender, not the resolved owner. This means the operator (caller) provides funds for increasePosition and receives proceeds for decreasePosition/closePosition/claimPayout. This is the standard ERC721 operator pattern — the approved party acts on behalf of the owner and handles the economic side.

3. Sponsored position handling: In _processClaimPayout, the sponsored payout split sends userProfit to msg.sender (operator) and sponsorReturn to the sponsor. The test test_claimPayout_WIN_approvedOperator_gets_user_profit_sponsor_gets_principal explicitly verifies this behavior.

4. Router simplification: The custody pattern removal is clean — _takePosition/_returnPositionIfNeeded are replaced by _requirePositionOwner (ownership check only), and Core handles authorization via the approval mechanism. The router's onERC721Received still exists for the openPositionWithSwap flow where the position NFT is minted to the router.

5. Event signature stability: No event signatures changed. All trade events (PositionIncreased, PositionDecreased, PositionClosed, PositionClaimed, PositionSettled, TradeFeeCharged) retain their existing parameter types and order. Only the semantic meaning of the trader field changes (now actual owner instead of router).

6. Test coverage: Comprehensive regression tests cover setApprovalForAll and approve operator flows across increase, decrease, claim, and batch claim. Fee-policy trader attribution is verified via MockTraderFeePolicy. Router tests verify no spurious NFT transfers occur.