Conversation
…llection # Conflicts: # CHANGELOG.md # CLAUDE.md # SuperwallKit.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Important
The timestamp parser accepts only fractional-seconds ISO 8601, so a backend that emits 2026-01-01T00:00:00Z silently disables the entire feature. The collection endpoint is also hardcoded outside SuperwallOptions.NetworkEnvironment. Both are worth settling before release.
Reviewed changes — the single commit 4cca2f6 adding session-local device IP collection, across 7 files.
- New
DeviceIPCollectoractor — holds timestampedipV4/ipV6observations with a 15-minute lifetime, validates addresses withinet_pton, keeps each family independent, and rejects::ffff:-mapped v4 addresses as IPv6. - Best-effort IPv4 fetch — a fire-and-forget request to
https://v4.superwall-enrichment.com/api/v1/enrichon an ephemeralURLSessionwith 3 s timeouts, no API key and no user attributes, coalesced to one attempt per 15 minutes. DeviceHelperwiring —refreshIfNeeded()at the top ofgetEnrichment(), plus a record / strip / re-merge step ingetTemplateDevice()that removes the six IP keys from the enrichment dict and re-adds only still-fresh observations.- Docs and tests —
README.mdcontract, aCLAUDE.mdinvariants section, aCHANGELOG.mdentry, and three swift-testing cases covering family separation,inet_ptonvalidation and refresh coalescing.
⚠️ Nothing exercises the DeviceHelper strip-and-merge path, or a timestamp without milliseconds
DeviceIPCollectorTests covers the actor in isolation, but the actual integration — DeviceHelper.swift:1050-1054, where cached enrichment IP fields are stripped and fresh observations merged back — has no test. Every timestamp in the suite is generated with .withFractionalSeconds, so the parser strictness described inline is exactly the case the tests cannot catch.
Technical details
# Cover the DeviceHelper integration and the timestamp format boundary
## Affected sites
- `Tests/SuperwallKitTests/DeviceIPCollectorTests.swift:10` — the only timestamp shape under test is `[.withInternetDateTime, .withFractionalSeconds]`, which is also the only shape the implementation can parse. The test and the bug agree with each other.
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:1050-1054` — untested. `DeviceHelperTests.swift` already builds a `DeviceHelper` and calls `getTemplateDevice()` (lines 219, 234), so there is an existing seam to extend.
## Required outcome
- A test that feeds a second-precision timestamp (`2026-01-01T00:00:00Z`) through `record` and asserts the observation survives. This test must fail against the current implementation.
- A test asserting that a stale `ipAddress` present in `enrichment.device` does not appear in the dictionary returned by `getTemplateDevice()`, and that a fresh observation does.
## Open questions for the human
- Is `DeviceHelperTests` the right home for the integration case, or should `DeviceIPCollector` be injectable into `DeviceHelper` so the fetch can be stubbed there too? It is currently a hardcoded `private let` at `DeviceHelper.swift:17`.ℹ️ PrivacyInfo.xcprivacy is unchanged while the SDK starts retaining and republishing the public IP
The manifest at Sources/SuperwallKit/Resources/PrivacyInfo.xcprivacy declares only NSPrivacyCollectedDataTypePurchaseHistory. This PR is the first point where the SDK's own code issues a request whose sole purpose is learning the IP, holds it for 15 minutes, and republishes it as named device attributes that flow into later requests and into the public getDeviceAttributes() surface. Worth a deliberate call rather than an omission — URLSessionConfiguration.ephemeral and inet_pton are confirmed not required-reason APIs, so only NSPrivacyCollectedDataTypes is in question.
Technical details
# Decide whether the new IP retention changes the privacy-manifest obligation
## Affected sites
- `Sources/SuperwallKit/Resources/PrivacyInfo.xcprivacy` — unchanged by this PR; `NSPrivacyCollectedDataTypes` lists only purchase history.
## Context
Apple's app-privacy guidance (https://developer.apple.com/app-store/app-privacy-details/) draws the line at retention: data "sent on a server call and not retained" needs no disclosure, whereas "you collect and store IP address from your users" does, mapped onto whichever data type matches the use. `DeviceIPCollector` retains for `lifetime = 15 * 60` and `DeviceHelper.getTemplateDevice()` re-emits the value into subsequent enrichment and audience-evaluation payloads.
## Required outcome
- An explicit decision, recorded somewhere durable, on whether `NSPrivacyCollectedDataTypes` needs a new entry.
## Open questions for the human
- The existing enrichment endpoint already returns IP-derived geo that the SDK persists via `LatestEnrichment`, so part of this may be a pre-existing question rather than one this PR creates. Does Superwall already have a position on this?ℹ️ Nitpicks
DeviceIPCollector.swift:21—lastAttemptis set before the fetch is attempted, so a single DNS blip or timeout burns the whole 15-minute window rather than just a successful attempt. ResettinglastAttempton failure would let the nextgetEnrichment()retry.DeviceIPCollector.swift:1— the file has no header comment. Every neighbour inNetwork/Device Helper/opens with the house// File.swift/// Created by …block.DeviceIPCollector.swift:23, 70— both failure paths (try?swallowing the fetch error, the unreachableURLguard) are silent. The repo convention is to route network failures throughLogger, which would make a misconfigured or undeployed endpoint diagnosable in the field.DeviceIPCollectorTests.swift:33, 42—coalescesRefreshesWithoutWaitingForNetworkgives a 100 ms fetch a 150 ms window, which is a narrow margin on a loaded CI machine. An actor-based signal (e.g. awaiting a continuation the stub resumes) would make it deterministic.
Claude Opus | 𝕏
There was a problem hiding this comment.
ℹ️ Merge-only update — no new findings. Prior review still applies.
Reviewed changes — the delta since the previous review at 4cca2f6 is a single commit, afd70c9, which merges origin/develop into the branch. It carries no changes of its own.
- Merge is semantically clean —
git diff --merge-base origin/developonDeviceHelper.swiftstill resolves to exactly the three hunks this PR introduced (theipCollectorproperty,refreshIfNeeded()at the top ofgetEnrichment(), and the strip-and-merge block ingetTemplateDevice()). The range-diff shows no change to the PR's own content; the churn isproject.pbxprojregeneration and context shift from files that landed ondevelop. - No collision with what
developbrought in —ipV4/ipV6/ipAddressappear nowhere in the merged tree outside this PR's own two files, and the newDeviceIdentifiers.swift/AttributionFetcherwork operates on user attributes, not the device dictionary this PR edits.
One note on merged-in context rather than a new finding: develop landed SuperwallOptions.devServer with localhost discovery in this same window. That sharpens the hardcoded-host point from the previous review — a superwall dev session now has a first-class local story, but the IPv4 collector will still reach out to production v4.superwall-enrichment.com from it.
Claude Opus | 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
ℹ️ Release-plumbing update — no new findings. The prior review's open items still apply.
Reviewed changes — the delta since the previous review at afd70c9 is a single commit, 4aa3b70, which stages the release. It has no behavioral surface.
- Version bumped to
4.18.0—Sources/SuperwallKit/Misc/Constants.swift:21andSuperwallKit.podspec:4. - Changelog entry regrouped — the IP-attributes bullet moved out from under
## 4.17.0into a new## 4.18.0→### Enhancementssection, which addresses the changelog nit from the previous review (thread now resolved). - Nothing else moved —
DeviceIPCollector.swift,DeviceHelper.swift,README.md,CLAUDE.md,DeviceIPCollectorTests.swiftandproject.pbxprojare byte-identical toafd70c9.
The version choice checks out against the repo's own rule in CLAUDE.md: origin/master and origin/develop were both on 4.17.0 and master's changelog already carries the full ## 4.17.0 section, so starting a new section and bumping all three files is correct rather than appending to a staged release. A minor bump fits an additive device-attribute change, and project.yml carries no hardcoded version, so the three-file set is complete.
The three substantive threads from the initial review are untouched by this commit and remain open — the fractional-seconds-only timestamp parser (DeviceIPCollector.swift:92-97), the hardcoded collection host outside NetworkEnvironment (DeviceIPCollector.swift:70), and the unconditional ipAddress strip (DeviceHelper.swift:1082-1084). Since this commit stages a release, the first of those is worth settling before the tag: if the backend from superwall/paywall-next#4160 ever emits a second-precision timestamp, the shipped feature is a silent no-op with no log to diagnose it.
Claude Opus | 𝕏
…llection # Conflicts: # CHANGELOG.md # SuperwallKit.podspec
- Parse observation timestamps with or without milliseconds, reusing two formatters. - Take the IPv4 host from the network environment, and skip the request outside release builds since there is no dev host. - Stop removing `ipAddress` from device attributes, since the enrichment API returns it today. - Retry after a failed fetch instead of waiting out the 15 minute window, and log the failure. - Ignore non-string values in the response rather than failing to decode. - Test the DeviceHelper stale-IP filtering, second-precision timestamps and retry, without timing sleeps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found. The three substantive threads from the initial review are addressed, and the new tests exercise the exact cases that were previously uncovered.
Reviewed changes — the delta since the previous review at 4aa3b70: a merge of origin/develop (07bdda1) and the feedback commit 1372760.
- Parsed timestamps with or without milliseconds — two stored
static letformatters sit behindDeviceIPCollector.date(from:), which also removes the per-access formatter allocation on thegetTemplateDevice()hot path. - Routed the IPv4 host through
NetworkEnvironment—ipV4EnrichmentHostresolves only for.release/.releaseCandidate,Api.Enrichment.ipV4Urlbuilds the URL, and anilURL makesrefreshIfNeeded()a no-op..developer,.localand.customnow send no traffic to the production domain. - Stopped stripping
ipAddress— only the four newipV4/ipV6keys are filtered, so any existingdevice.ipAddressaudience filter keeps working. The README states the pass-through. - Retried after a failed fetch —
lastAttemptis cleared on failure (guarded so it cannot clobber a newer attempt) and the error goes throughLogger. - Made
DeviceIPCollectorinjectable intoDeviceHelper— addedtemplateDevice_dropsStaleIPObservations, which would fail without the strip, plus collector tests for second-precision timestamps, newer-wins, retry-after-failure, the environment → URL mapping, and tolerantparseDevice. The coalescing test now awaits the returned task instead of racing a sleep. - Rebased the changelog onto develop's staged
4.18.0— develop already carries the bump, so the PR only appends its bullet to the existing### Enhancements, asCLAUDE.mdrequires when a release is already staged.
The privacy-manifest question from the initial review is still a decision for the team rather than a code issue, and the online docs checkbox is still open. Neither blocks this change.
Claude Opus | 𝕏
IP collection is off by default and runs only when config has the `enable_mmp` toggle on. With it off, no IPv4 request is made and no `ipV4`/`ipV6` attributes are exposed. The check happens when building device attributes, so a first launch picks it up once config arrives. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Important
The MMP gate is correct, and it answers the earlier privacy-manifest question. But moving refreshIfNeeded() onto the getTemplateDevice() hot path, together with the failure reset added last round, means any persistent fetch failure sends a new IPv4 request on almost every device-attribute read. Details inline.
Reviewed changes — the delta since the previous review at 1372760 is one commit, 363faa3, which puts IP collection behind the backend's enable_mmp toggle.
- Added
FeatureFlags.enableMMP— decoded from theenable_mmptoggle, defaulting tofalse, encoded back inencode(to:), andtruein the stub. Old cached configs without the key decode to off. - Gated collection in
getTemplateDevice()— the fouripV4/ipV6keys are still always stripped.refreshIfNeeded(),recordand the merge now only run when the flag is on. The refresh moved here fromgetEnrichment()because the first cold-launch enrichment runs before config is loaded. - Widened
DeviceHelper's factory toFeatureFlagsFactory—DependencyContaineralready conforms, and it is the only production call site. - Added flag tests —
templateDevice_withoutMMPFlag_skipsIPCollectionchecks that no attributes are exposed, and thatrefreshIfNeeded()still returns a task afterwards, which proves no lookup had started. This test can fail.mmpFlagIsOffUnlessTheBackendTurnsItOncovers decoding. - Updated docs — the README says collection is off by default, and
CLAUDE.mdrecords that apps turning on the MMP declare the IP collection in their own privacy manifest.
ℹ️ Nitpicks
README.md:119-120— "limited to one per 15 minutes when enrichment runs" no longer matches the code. Attempts now start on any device-attribute read while the MMP flag is on, not only during enrichment.
Claude Opus | 𝕏
Moves the IP collection switch from a general toggle to `attributionOptions.mmp.enabled`, next to Apple Search Ads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
ℹ️ The gate swap is clean, and I found no new code issues. The retry-storm thread on
DeviceHelper.swift:1091is still open, sinceDeviceIPCollector.swiftdidn't change in this commit. The README nit from the last review also still applies.
Reviewed changes — the delta since the previous review at 363faa3 is one commit, a5f1a01. It moves the MMP switch from a feature flag to the attribution config options.
- Reverted
FeatureFlags.enableMMP— theenable_mmptoggle is gone from decode, encode, the memberwise init and the stub.FeatureFlags.swiftis no longer in the PR. - Added
Attribution.mmp— a new optionalMMPAttribution { enabled: Bool }sits next toappleSearchAds. Its init defaultsmmptonil, so the existingConfig.stub()call site still compiles. It decodes fromattributionOptions.mmpthrough the existingConfig.CodingKeys.attributionmapping. - Changed the gate to read
ConfigStateFactory—getTemplateDevice()now checksmakeConfigState().value.getConfig()?.attribution?.mmp?.enabled == true, the same shapeAttributionPosteruses forappleSearchAds.DependencyContaineralready conforms, and it's the only production factory. - Updated tests and docs —
mmpFlagIsOffUnlessTheBackendTurnsItOndecodesAttributionwith and withoutmmp.setMMPFlagbuilds the config through the new init.README.mdandCLAUDE.mdnow nameattributionOptions.mmp.enabled.
ℹ️ attributionOptions.mmp.enabled gates IP collection but not the MMP install match
The SDK now decodes a switch named for the whole MMP. Only IP collection reads it: MMPAttributionManager still sends the install match to the /mmp endpoint whatever the flag says. By contrast, appleSearchAds.enabled does gate AttributionPoster. The doc comment on the new field, "Superwall's install attribution (MMP). Off unless the backend enables it.", suggests it gates more than it does.
Technical details
# Decide what `attributionOptions.mmp.enabled` is meant to gate
## Affected sites
- `Sources/SuperwallKit/Config/Models/Attribution.swift:12-13` — the doc comment describes `mmp` as the install-attribution switch.
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:1090` — the only reader of `attribution?.mmp?.enabled`.
- `Sources/SuperwallKit/Analytics/Attribution/MMPAttributionManager.swift` — the install match doesn't check the flag.
## Required outcome
- Either the doc comment says the field gates device IP collection only, or the install match honors the flag too (that would be a follow-up, not this PR).
## Open questions for the human
- On the backend, does `attributionOptions.mmp.enabled` mean "this app uses the Superwall MMP", with the install match expected to stop when it's off? Or is it only meant to control IP collection?Claude Opus | 𝕏
The lookup now runs whenever device attributes are read, so clearing the last attempt on failure meant an outage or offline device retried on almost every read. Wait 60 seconds after a failure instead; a success still waits the full 15 minutes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found. The retry backoff fixes the hot-path retry storm, and the new test fails against the old behavior.
Reviewed changes — the delta since the previous review at a5f1a01 is one commit, 168ae34. It adds a failure backoff to the IPv4 lookup.
- Replaced the window check with
nextAttemptAt— starting an attempt sets it todate + lifetime, and a failure pulls it in todate + retryDelay(60 s). ThelastAttempt == dateguard still stops a stale failure from overwriting a newer attempt. A persistent failure (offline, v4 host down, blocked host) now costs at most one request a minute instead of one on nearly everygetTemplateDevice()read. - Rewrote the retry test as
waitsAMinuteBeforeRetryingAFailedFetch— it uses an injectableClockto check that there's no retry at +30 s, a retry at +61 s, and the full 15-minute window after a success. The +30 snilassertion would fail against the previous clear-on-failure code, so this test can catch the regression. - Updated the README timing line — it now says a lookup can start on any device-attribute read while the MMP is on, at most every 15 minutes, or a minute after a failure. This fixes the stale-wording nit from the
363faa3review.
The scope question from the last review is still open for the team: attributionOptions.mmp.enabled gates IP collection but not the MMPAttributionManager install match. It doesn't block this change.
Claude Opus | 𝕏
On a cold launch the first enrichment is read before config arrives, so its IPs were dropped and the IPv4 lookup waited for a later read. Enrichment IPs are now always kept in memory (only exposing them is gated), and config arriving with the MMP on starts the lookup. An ipV6 without its own timestamp could also borrow the IPv4 ipAddressObservedAt and look fresh; each address now only uses its own timestamp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found. Both cold-launch findings from the last round are fixed, and each fix comes with a test that fails against the old code.
Reviewed changes — the delta since the previous review at 168ae34 is one commit, 6220829. It starts the IPv4 lookup when config is applied and stops one IP family from borrowing the other family's timestamp.
- Started the lookup when config is applied —
ConfigManager.processConfignow calls a newstoreAndApply(_:), which contains the same four save/trigger/variant lines as before plusdeviceHelper.startIPCollectionIfEnabled(for:). That method reads the flag from the incoming config, not fromconfigState, so it works before config is published.processConfigis the only path that publishes.retrieved, so cold sync, cold cached andrefreshConfigurationare all covered.Config.stub()has nommp, so the existingConfigManagertests don't start lookups. - Recorded enrichment IPs even when the flag is off —
recordnow runs outside the MMP gate ingetTemplateDevice(), butattributes()is still only merged when the flag is on. If a later enrichment replaces a pre-config one, its observations are kept, and nothing new is exposed while the MMP is off. These IPs were already in memory throughenrichment. - Paired each address only with its own timestamp — when
ipV{n}is present,recordonly usesipV{n}ObservedAt. It falls back toipAddress+ipAddressObservedAtonly when the family key is missing, so anipV6without a timestamp is dropped instead of taking the IPv4 time. - Added tests —
doesNotPairAnAddressWithAnotherFamilysTimestampandcoldLaunch_keepsEnrichmentIPsAndStartsLookupWhenConfigArriveswould both fail against168ae34.configWithMMPOff_doesNotStartLookupcovers the off path.templateDevice_withoutMMPFlag_skipsIPCollectionlost itsattributes().isEmptycheck, which is correct becauserecordis no longer gated. It still checks that no attributes are exposed and no lookup starts.
The scope question from earlier rounds is still open for the team: attributionOptions.mmp.enabled gates IP collection but not the MMPAttributionManager install match. It doesn't block this change.
Claude Opus | 𝕏

Changes in this pull request
Add a best-effort IPv4 request alongside existing enrichment and expose
ipV4,ipV6,ipV4ObservedAt, andipV6ObservedAtin device attributes. Each family is retained separately; malformed or stale observations are omitted. Collection does not wait on the network during configuration or purchases, attempts are coalesced for 15 minutes, and the public IPv4 request sends no user attributes or API key.This is off by default. It only runs when the backend turns on
attributionOptions.mmp.enabledin the app's config, so the SDK's privacy manifest stays as it is and apps that use the MMP declare the IP collection themselves. The IPv4 request is only made with the release network environments, since the IPv4-only host has no dev version. AnyipAddressthe enrichment API returns passes through unchanged.IPv6 is taken from the existing enrichment response when that connection uses IPv6. Deploy https://github.com/superwall/paywall-next/pull/4160 before releasing this change. It does not guarantee IPv6 on dual-stack devices. Mobile wrappers inherit this when they adopt the native release; no wrapper versions are bumped here.
Validation: 29 tests passed on the iOS 26.4 simulator (
DeviceIPCollectorTestsandDeviceHelperTests). Full SDK and test targets compiled. Ranscripts/lint.sh; no new-file violations remain (existing repository/configuration warnings remain). Added README contract and changelog entry. UI/demo/Catalyst/visionOS and public online docs remain unchecked.Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.swiftlintin the main directory and fixed any issues.The PR should not merge until IP collection can run after the MMP flag becomes available on a cold launch.
Findings
Fix with agent prompt
Summary
The PR adds an MMP-gated, best-effort IPv4 lookup and exposes separately timestamped IPv4 and IPv6 device observations. The initial configuration path reads device attributes before publishing the flag, however, so collection can be missed on a cold launch.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Fetch config] --> B[Read enrichment and device attributes] B --> C{MMP flag readable?} C -->|No on cold launch| D[Skip IP collection] D --> E[Publish fetched config] E --> F[No guaranteed subsequent attribute read]Reviews (1) · Last reviewed commit: "Wait a minute before retrying a failed I..."