fix(v3)!: nullable hardening + repair the broken input-token-permit path - #80
Merged
Conversation
Nullable-reference hardening ahead of 1.0.0. Option/params types now carry explicit null contracts derived from the upstream TS interfaces (required field -> 'required', 'x?: T' -> nullable). Solution now builds with ZERO compiler warnings (was 106). Root causes fixed, not suppressed: - CurrencyAmount.Wrapped()/.AsBaseCurrency() were declared nullable but provably never return null; tightening them cleared all 5 downstream CS8601/CS8602 flow warnings at once. - Route._midPrice is a genuine lazy cache -> now Price<,>?. Real bug found by the pass: V3 SwapRouter's input-token-permit path was UNUSABLE. Staker.SwapOptions.InputTokenPermit was typed to an empty stub class, so the only assignable value failed SelfPermit.EncodePermit's type tests and always threw 'Invalid permit options' -- and the valid permit types couldn't be assigned at all. Upstream ships no test for this path, so the port inherited the gap. Modelled the upstream 'PermitOptions = StandardPermitArguments | AllowedPermitArguments' union as SelfPermit.IPermitOptions and typed every permit option against it (was 'object?'), making misuse a compile error. Pinned by a new regression test. Also: PoolTests.BigNums_CorrectlyHandlesTwoBigIntegers now awaits GetInputAmount -- the unawaited Task swallowed exceptions, so it could pass vacuously. 1695 tests green; dotnet format clean. BREAKING: option initializers must supply now-'required' fields; permit options are typed as SelfPermit.IPermitOptions instead of object.
SummarySummary
CoverageUniswapSharp - 85.2%
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-1.0.0 hardening pass. Solution now builds with zero compiler warnings (was 106). 1,695 tests green,
dotnet formatclean.The headline: a real, fully-broken public API path
The pass surfaced that V3
SwapRouter's input-token-permit feature could never work:Staker.SwapOptions.InputTokenPermitwas typed asStaker.PermitOptions— an empty stub class (// Implement PermitOptions).SelfPermit.EncodePermittookobjectand runtime-tested forIStandardPermitArguments/IAllowedPermitArguments.PermitOptionssatisfies neither → falls toelse→ always throws"Invalid permit options".CS0029), so there was no way to use it correctly.The
objectparameter is what hid this from the compiler. Upstream ships no test for this path, which is why the port inherited the gap.Fix: model the upstream union
PermitOptions = StandardPermitArguments | AllowedPermitArgumentsas a marker interfaceSelfPermit.IPermitOptions, implemented by both argument interfaces. Every permit option is now typed against it instead ofobject—Staker.SwapOptions.InputTokenPermit,NonfungiblePositionManager.AddLiquidityOptions.Token0Permit/Token1Permit,Router.SwapOptions.InputTokenPermit,SwapAndAddOptions.OutputTokenPermit— so misuse is a compile error. Empty stub deleted. Pinned by a new regression test (V3/SwapRouterPermitTests.cs).Nullable hardening
Null contracts derived from the upstream TS interfaces: upstream-required (
x: T) →required; upstream-optional (x?: T) → nullable. Applied acrossNonfungiblePositionManager(24),Staker(11),SelfPermit(4),Payments(2).Two root causes fixed rather than suppressed:
CurrencyAmount.Wrapped()/.AsBaseCurrency()were declared nullable but provably never return null (every path constructs via the non-nullableFromFractionalAmount;BaseCurrency.Wrapped()returns a non-nullableToken). Tightening them cleared all five downstreamCS8601/CS8602warnings at once and strengthens the contract for callers.Route._midPriceis a genuine lazy cache → nowPrice<,>?.A test that could pass vacuously
PoolTests.BigNums_CorrectlyHandlesTwoBigIntegerscalledGetInputAmountwithout awaiting (faithfully copying upstream). In C# an unawaitedTaskswallows exceptions — so the test would have gone green even ifGetInputAmountthrew on the very big numbers it's named for. Now awaited; it passes, which actually proves the behaviour.required.SelfPermit.IPermitOptionsinstead ofobject.Both are source-level tightenings that are far cheaper now than after 1.0.0 — which is exactly why this ran before cutting stable.