Repository navigation
Conversation
685a89e to
a06a143
Compare
|
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Aligns the Pubky auth sheet with the Figma frames: names the requester in the lead and success sentences, adds the DETAILS and BEFORE YOU CONTINUE headings, and switches to the compact profile row. Reviewed a06a143, full tier (an approval sheet is a security path), reasoned from the code and CI: iOS does not build on this box.
What I checked, and 3 candidates I ruled out
Read in full: PubkyAuthApprovalSheet.swift; traced config.request.clientID back to Paykit.parsePubkyAuthUrl in PubkyAuthRequest.swift; t(_:variables:) in LocalizeHelpers.swift
CI: validate (translations), Greptile and change detection green; Run Tests, Run Integration Tests and build-local were still pending at review time
Ruled out
- Requester id lost from the screen: the separate
Requester IDline goes, but the id now appears in the lead sentence whenever it is non-empty, and the fallback sentence covers the empty case, so a user still sees who is asking before approving. - Removed string keys still referenced: no Swift reference to
pubky_auth__requesterorpubky_auth__paykit_access_titleremains, and the other localizations carry no copy of them. - Missing illustration:
coin-stack-4.imagesetexists inAssets.xcassets/Illustrations.
Merge confidence: 4/5, no findings and copy or layout a unit test would not apply to, but the test and build jobs were still pending and the branch was not built here.
|
Two independent reviews. worth doing, does not block
nits
the reviewers disagree, your call
|
|
Went through the review.
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3d58d12 to
12e44e9
Compare
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the PR diff from merge base 33982a09, at 12e44e9.
2 actionable findings — resolve or provide an evidence-backed rebuttal.
The named authorize and success sentences can rewrite a requester id or service name that contains another {placeholder}. Paykit accepts those strings, and this sheet no longer has a separate requester line. The matching Android sheet in bitkit-android#1421 at 1dcb6ad formats with %s and leaves the same strings unchanged. Accent-tag removal matches AccentedText, but the new unit test is nested inside XCTAssertThrowsErrorAsync and is not an XCTest method.
Validation: Run Tests passed on 12e44e9. This review did not rerun that suite. Device testing was not performed. The Figma frames linked on the PR were not re-rendered here. iOS still shows the authorization relay block and uses the shared intro sizing; that is intentional in this PR.
Recommended before device testing: show placeholder-bearing requester ids and service names literally, and attach the accent-tag test to PubkyAuthApprovalSheetTests.
Suggested additional test cases
- iOS, fresh wallet, valid auth URL with
cid={service}andcaps=/pub/paykit/:rw. Open the authorize sheet. The lead sentence shows{service}as the requester, notpaykit. - Same setup with
cid=bank.comandcaps=/pub/{clientId}/:rw. The service portion shows{clientId}, and the permission row shows/pub/{clientId}. - Run
testRequestTextLosesAccentMarkupEvenWhenTagsAreNestedas a method ofPubkyAuthApprovalSheetTests. The nested-tag assertions execute and pass.
Findings
- [MEDIUM] Consent sentence rewrites requester ids that contain placeholders — inline at
Bitkit/Views/Sheets/PubkyAuthApproval/PubkyAuthApprovalSheet.swift:333. - [LOW] Accent-tag test is nested where XCTest will not run it — inline at
BitkitTests/PubkyAuthApprovalSheetTests.swift:625.
| t("pubky_auth__description_prefix") + "<accent>" + serviceText + "</accent>" + t("pubky_auth__description_suffix"), | ||
| requesterText.isEmpty | ||
| ? t("pubky_auth__description_prefix") + "<accent>" + serviceText + "</accent>" + t("pubky_auth__description_suffix") | ||
| : t("pubky_auth__description_named", variables: ["clientId": requesterText, "service": serviceText]), |
There was a problem hiding this comment.
[MEDIUM] Consent sentence rewrites requester ids that contain placeholders
Trigger: a Paykit auth URL whose client id or service path contains another consent placeholder, such as cid={service} with caps=/pub/paykit/:rw, or cid=bank.com with caps=/pub/{clientId}/:rw. ClientId::new in pubky-common 0.15.0 (paykit-rs 2ea4b2b) accepts any non-empty string up to 253 characters, and storage paths allow { in a segment.
Mechanism: descriptionText and successDescriptionText pass those values through t(), which replaces {clientId}, {service}, and {pubky} with replacingOccurrences and then keeps scanning the result for the remaining keys. Dictionary order is not fixed. pubkyAuthLiteralText only removes <accent> tags, so a value such as {service} is inserted and can be replaced by the service name. The same pass can turn a service name of {clientId} into the client id, and on the success sentence a client id containing {pubky} can include the truncated pubky.
Consequence: this path would name a different requester or service than the grant. The lead sentence is now the only client-id display, so a cid of {service} can read as paykit while the authorized client id remains {service}. The permission row still shows the raw path, but not the client id.
Expected behavior: show the requester id and service name literally, including {service}, {clientId}, and {pubky}. Substitute each template placeholder from the original string only, without scanning inserted values. Android already does that with positional %s arguments.
Evidence basis: source analysis of t() and the two call sites at 12e44e9, plus the same replacement sequence checked outside the app. The iOS UI was not run. Run Tests does not cover this input.
| XCTFail("Expected expression to throw", file: file, line: line) | ||
| } catch {} | ||
|
|
||
| func testRequestTextLosesAccentMarkupEvenWhenTagsAreNested() { |
There was a problem hiding this comment.
[LOW] Accent-tag test is nested where XCTest will not run it
Trigger: PubkyAuthApprovalSheetTests runs in CI or locally.
Mechanism: testRequestTextLosesAccentMarkupEvenWhenTagsAreNested is declared inside the file-private function XCTAssertThrowsErrorAsync, after PubkyAuthApprovalSheetTests has already closed. Nothing calls that nested function. XCTest only runs methods on the test class, so the three XCTAssertEqual checks are never executed. The function still typechecks, which is why the suite can pass.
Consequence: a regression in pubkyAuthLiteralText would not fail CI. The PR presents this test as coverage for nested <accent> tags.
Expected behavior: declare the test as a method on PubkyAuthApprovalSheetTests so the nested-tag cases run with the class.
Evidence basis: source analysis of BitkitTests/PubkyAuthApprovalSheetTests.swift at 12e44e9. The suite was not rerun here.
iOS port of synonymdev/bitkit-android#1421.
This PR aligns the Paykit authorization sheet with the Auth (paykit) flow in Figma.
Description
coin-stack-4).app.paykit.server is requesting permission…) and removes the separateRequester IDline. The old sentence stays as the fallback when a request carries no client id.REQUESTED PERMISSIONS,DETAILSandBEFORE YOU CONTINUEheadings with 32pt between sections, removes the divider under the permissions, and uses the Figma copy for the Paykit details and trust warning.<accent>tags in them are removed before the sentence is styled, so a request cannot inject its own emphasis.Out of Scope
PubkyAuthApprovalSheet.swift: theAUTHORIZATION RELAYsection and the relay line on the Earn screen are iOS additions that the Figma frames do not show; they are kept.SheetIntro.swift: the Earn screen keeps the shared intro component's sizing and side insets instead of the frame's.Localizable.strings: the Figma title reads "Authorization Succesful"; the correctly spelled title is kept. New and changed strings are English only.Authorize paykit FaceIDframe: Face ID is the system prompt and is unchanged.Design
Preview
iPhone 17 simulator, fresh wallet and Bitkit-created Pubky profile, combined
paykit-access-v1.watch-only-account-v1claim.QA Notes
Journeys
N/A — no journey added or updated. The route and identifiers are unchanged, so
wallet-leg.xmlandpaykit-only-approval.xmlstill drive this sheet as written.Manual Tests
bitkit://pubky-auth/setupURL withcaps=/pub/paykit/:rw, acidandx-bitkit-claim=paykit-access-v1.watch-only-account-v1→ the Earn, Authorize, Authorizing and Success screens match the image above — needs a valid auth URL fixture.Automated Checks
PubkyAuthApprovalSheetTests.swift— accent tags in a requester id are removed, including nested onesPubkyAuthApprovalSheetTests,swiftformat --lintandnode scripts/validate-translations.js— pass locally; a 253-character and a tag-wrappedcidwere checked on the iPhone 17 simulator.