Repository navigation
Conversation
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
a2e9569 to
18cbbec
Compare
18cbbec to
355a467
Compare
84f5432 to
ed9dc03
Compare
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>
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>
ed9dc03 to
1dcb6ad
Compare
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete pull request diff against merge base 50c9bc34, at 1dcb6ad. No earlier assessment baseline was available.
2 actionable findings — resolve or provide an evidence-backed rebuttal.
The grant, local unlock, and capabilities path are unchanged. Consent copy matches the linked Figma frames and the comparison revision 12e44e9 on bitkit-ios#880. wallet-leg.xml, paykit-only-approval.xml, and paykit-reconnect.xml still match the Paykit details sentence and the existing test tags. An earlier note about a non-scrolling footer is already addressed here: the warning and profile scroll with the details when the column is taller than the sheet (thread).
Validation: Inspected PubkyAuthApprovalSheetTest (accent-tag stripping only). I did not run unit tests or a device. On this commit, GitHub CI build, lint, and the local e2e matrix 37923940835 succeeded. Those e2e jobs do not open this sheet. Device testing: not performed in this review. iOS was comparison-only, not a full review.
Non-blocking consistency suggestions: The Authorize frame stacks Before you continue and the profile directly under the details, with the spare space above the buttons. This sheet, like iOS, expands a spacer so that group sits on the bottom when the content is short. iOS still joins every service name with "and"; Android still shows only the first. The success check is scaled uniformly in a 256dp slot; the Authorized frame places a 274×206 image inside a 256×192 frame.
Suggested additional test cases:
- Android, dev build,
pubkyauth://signin_grantwhosecidis longer than the sheet text width and has no spaces: open Authorize and Success. The requester is fully visible or ellipsized, and the permission sentence stays fully visible. - Android, same flow, with a
cidthat contains an encoded newline plus a second sentence, and one that contains a bidi override: the sheet shows one consent sentence in the app's word order, with no extra line. - Android earn consent on a phone-width sheet: the coin stays clear of the title, matching the Earn frame.
Findings
- [MEDIUM] Isolate requester text in the consent sentence — inline at
app/src/main/java/to/bitkit/ui/screens/profile/PubkyAuthApprovalSheet.kt:510. - [LOW] Clip the earn coin to its illustration frame — inline at
app/src/main/java/to/bitkit/ui/screens/profile/PubkyAuthApprovalSheet.kt:298.
| } else { | ||
| stringResource(R.string.profile__auth_approval_service, service) | ||
| } | ||
| BodyM(text = text.withAccentBoldBright(), color = Colors.White64) |
There was a problem hiding this comment.
[MEDIUM] Isolate requester text in the consent sentence
Trigger: a sign-in grant whose cid is a long domain, or contains an encoded newline or bidi control. ClientId::new in pubky 0.15.0, which paykit-rs v0.1.0-rc71 uses, accepts any non-empty string up to 253 characters, and parse_client_id stores that value.
Mechanism: DescriptionText and SuccessDescriptionText only strip <accent> tags, then interpolate the requester into a multiline BodyM. A newline becomes another line inside the consent copy. A bidi override can reorder that sentence. A long id has no line-break opportunities, and BodyM clips overflow instead of ellipsizing, so the id is cut off with no marker. The same helper is used for the service name.
Consequence: the lead sentence can show attacker-written lines, a reordered permission statement, or a truncated requester, while Authorize still grants that exact client id. The separate permission rows stay accurate, but the sentence that names the requester does not.
Expected: render the requester and service as single-line literal runs, with bidi isolates, newlines and other controls removed or replaced, and ellipsis when they do not fit, so they cannot add lines, reorder the sentence, or hide the rest of the id.
Evidence basis: source analysis at 1dcb6ad. Parser checked in pubky v0.15.0 (ClientId::new, parse_client_id) as pinned by paykit-rs v0.1.0-rc71, matching paykit-android 0.1.0-rc71. The unit test only covers accent tags. Not executed on a device.
| .align(Alignment.CenterHorizontally), | ||
| .align(Alignment.CenterHorizontally) | ||
| .graphicsLayer { | ||
| scaleX = COIN_SCALE |
There was a problem hiding this comment.
[LOW] Clip the earn coin to its illustration frame
Trigger: the watch-only earn step on a phone-width sheet.
Mechanism: the coin is laid out at 256dp and then scaled by 329/256 from the center and shifted 7dp down, with clipping left off. The painted height is about 329dp, so roughly 43dp extends below the slot. The spacer under the image is 32dp, and the headline is drawn immediately after that, so the art meets the title. In the Earn frame the raster is 329×247 and sits inside a 256×192 illustration frame in the 256 slot, with the title below that slot. The comparison iOS sheet at 12e44e9 does not apply this scale; SheetIntro uses aspect-fit with a 256pt max height.
Consequence: the coin collides with the earn headline instead of leaving the gap shown in the frame.
Expected: size and clip the asset to that illustration frame so the title stays clear of the art.
Evidence basis: source analysis at 1dcb6ad, compared with Figma node 49203:235173 and iOS SheetIntro at 12e44e9. Not executed on a device.
This PR aligns the Paykit authorization sheet with the Auth (paykit) flow in Figma.
Description
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 32dp between sections, removes the divider under the permissions, uses the Figma copy for the Paykit details and trust warning, and sets both to the 15sp body style.<accent>tags in them are removed before the sentence is styled, so a request cannot inject its own emphasis.Out of Scope
strings.xml: the Figma title reads "Authorization Succesful"; the existing correctly spelled "Authorization Successful" is kept.Authorize paykit FaceIDframe: the Face ID prompt is the iOS system dialog; Android keeps its own biometric/PIN prompt.Type.kt: body styles keep the app-wide 0.4sp letter spacing where the frames show 0.bitkit-ios: ported in fix: align paykit auth sheet with figma bitkit-ios#880.Design
Preview
Pixel 9 emulator, dev build, combined
paykit-access-v1.watch-only-account-v1claim.QA Notes
Journeys
N/A — no journey added or updated. The route and test tags are unchanged, so
wallet-leg.xmlandpaykit-only-approval.xmlstill drive this sheet as written.Manual Tests
pubkyauth://signin_grantURL withcaps=/pub/paykit/:rw, acidandx-bitkit-claim=paykit-access-v1.watch-only-account-v1→ the Earn, Authorize, Authorizing and Success screens match the Figma frames above — needs a valid auth URL fixture.DETAILSsection — needs a valid auth URL fixture.Automated Checks
PubkyAuthApprovalSheetTest.kt— accent tags in a requester id are removed, including nested onesjust compile,just test,just lint— pass locally.