Repository navigation
feat(reputation): import reputation from lnp2pBot - #676
Merged
Merged
Conversation
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35c5de794a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4 tasks done
Catrya
requested changes
Oct 6, 2026
Catrya
left a comment
Catrya
left a comment
Member
There was a problem hiding this comment.
Review evidence
Decision: Request changes
Tested commit: 2615478f (PR head). I reviewed the PR's own diff against #675's head (a55bd4b1..2615478f). I also merged #675 and #676 locally onto main at 588ff66c (not pushed).
Environment:
- Harness: Neither. Plain
cargoandflutterin a git worktree; no Ortsom or Mortsom run. - Platform: Linux. Web compile-checked only (
wasm32-unknown-unknown). - Daemon source: Not applicable. No daemon here carries MostroP2P/mostro#1028–#1032.
- App source: Pull request #676, with #675 below it, merged onto current
main.
Commands and tests executed:
git merge --no-ff --no-commit origin/feat/reputation-import-uiontoorigin/main. 18 files conflict:- the six ARBs and the generated
app_localizations*.dart; lib/src/rust/api/orders.dart,lib/src/rust/frb_generated.dart,rust/src/frb_generated.rs;rust/Cargo.tomlandrust/Cargo.lock.
- the six ARBs and the generated
- I resolved them locally:
- the ARBs, by keeping both sides' keys;
Cargo.tomlandCargo.lock, withmain's side (mostro-core = "0.17.1");- the generated files, by regenerating with
./scripts/frb-generate.shandflutter gen-l10n.
- On the merged tree:
cargo test --lockedcargo clippy --locked -- -D warningscargo check --locked --target wasm32-unknown-unknownflutter analyzedart tool/design_check.dart --base origin/mainflutter test(full suite)
- Throwaway widget probes on
ImportReputationScreen:- Stale attestation: paste attestation A, tap Check, replace the text with something else, then tap Import.
- npub on screen: count any text containing
npub. - Rating format: Spanish locale, check the rendered figure.
- Layout: 320 dp wide, 2× text, German. Scroll the whole
ListViewbefore and after Check, checking for
- Rating format: Spanish locale, check the rendered figure.
- Layout: 320 dp wide, 2× text, German. Scroll the whole
ListViewbefore and after Check, checking for overflow. - Read the PR's own diff in full:
lnp2pbot.dart,reputation_api.dart,reputation_errors.dart,import_reputation_screen.dart,reputation_controls.dart, the ARBs and the tests. - Compared the hand-off with lnp2pBot/bot#933 (open): the start-parameter format and the bot's confirmation text.
- Checked how
mainshows the identity key:account_screen.dart.
Results:
- Passed:
cargo test --locked: 1129 passed, 0 failed, 30 ignored. That is withmostro-core0.17.1, not the PR's pin.- clippy and wasm check: clean.
flutter analyze: no issues.design_check: no breaks (5 files).flutter test: 2139 passed, including the PR's tests.- Layout probe: no overflow anywhere on the screen.
- The bot link matches bot#933, which expects
^/start(?:@\w+)? rep_([A-Za-z0-9_-]{43})$.
- Failed:
- Merge with
main: 18 conflicting files (see above). flutter test: 2 failures, bothattachment_bubbles_golden_test(dark and light). They fail the same way onmainin this environment, so they are not caused by this PR.- Stale-attestation probe: after the text was replaced, Import was still offered, and it sent attestation A (
imported=[{"id":"a",…}]). - npub probe: 0 texts containing
npubon the screen. - Spanish rating probe: renders
4.87, not4,87.
- Merge with
- Skipped: 4 in
flutter test, also skipped onmain. Web smoke test not run on the merge. - Flaky: None observed
Manual observations:
- bot#933 relies on the app to show the npub. The bot asks the user to confirm: "Check that it is the one your Mostro app shows. Once confirmed, your account here is bound to it". But the app shows the identity nowhere. On
mainthe Account screen exposes the public key only as a 1-px automation node (keysPublicKey), and this screen does not show it either. - No gate on what the node imports. The screen never reads
support().importIssuers. "Get it from lnp2pBot" and Check are offered even when the active node does not import. Check also accepts an attestation from an issuer the node does not trust, or addressed to another identity. All of these are refused only at Import. - Privacy mode is not checked. The screen does not read it.
_openBotsends the identity key to Telegram and lnp2pBot regardless, and Rust then refuses the import. #677 disables the button that opens this screen, but the screen itself does not guard. _openBotfails silently. It ignores afalsefromlaunchUrland only catches exceptions. It also does nothing visible when there is no identity or the key is not hex.- Codex's comments on this PR (2026-10-03) have no reply and no change. The last push was 2026-10-03 16:26.
Not tested or limitations:
- No import from this app against a daemon, and no round trip with a real lnp2pBot (bot#933 is open). The PR's own body says "Not run on a device yet", and it has no screenshots.
- No web build or smoke test on the merge.
- In the layout probe, I did not get the figures card itself on screen at 2× text.
- The stack was tested with
mostro-core0.17.1 frommain, not with the PR's pin.
Unresolved findings:
Blocking:
- Import can send an attestation other than the one in the field.
- After Check, editing or replacing the text does not clear
_attestation, so Import stays enabled and sends the previously checked attestation. - Reproduced above. Import is irreversible and links the two accounts.
- Fix: clear the verified state when the field changes, or re-validate the current text before importing. Codex P2, open.
- After Check, editing or replacing the text does not clear
- The user cannot check the npub that lnp2pBot asks them to confirm.
- bot#933's confirmation binds the user's lnp2pBot account and tells them to check the npub against what the app shows. The app shows none.
- Fix: show the identity npub, encoded in Rust (see the #677 review), next to "Get it from lnp2pBot".
- The node's import capability is learned only at the last step.
- On a node that does not import, the user is sent to lnp2pBot, binds their account there, pastes and checks, and is refused only at Import.
- An untrusted issuer, or an attestation addressed to another identity, also passes Check.
- Fix: when
importIssuersis null, say so up front and disable the bot button. At Check, run the local issuer and destination checks.
- No end-to-end run.
- The Manual testing steps were written but never executed: "Not run on a device yet", no screenshots.
- Maintainers are exempt from the contribution bar, but this flow ends in a binding on two external systems (the node and lnp2pBot).
- The stack cannot merge as it stands.
- It depends on #675 (changes requested).
- It inherits the
mostro-corepin to the deletedfeat/reputation-mergebranch. Switch to 0.17.1, which works (verified above).
Non-blocking:
- Silent bot button.
_openBotignores afalsefromlaunchUrland gives no feedback when there is no identity or the key is not hex. Codex P2, open. - Rating not locale-formatted.
attestation.ratingis shown raw (4.87in Spanish), which breaks DS-L10N-3 (MUST).order_list_item.dartalready usesNumberFormat('0.##', locale). - No plural forms.
reputationFigureshas no ICU plurals: "1 ratings received", "trading for 1 days". - No living contract.
specs/004-mostro-p2p-client/contracts/reputation.mddoes not describe the import flow. Codex P1, open; same finding as on #675. - Privacy mode. The screen does not check it, so
_openBotsends the identity key to Telegram and lnp2pBot even though the import will be refused. Mitigated by #677, not by this screen. - Tests:
- nothing covers the pending-attestation prefill;
- nothing covers the stale state in blocking finding 1;
- the DS-A11Y-4 test (320 dp, 2× text, German) that a new screen needs is missing. The behaviour holds in my probe, but no test pins it.
- Noise: an unrelated formatting change to the
lockEscrowroute inapp_routes.dart.
grunch
force-pushed
the
feat/reputation-transfer-core
branch
from
October 14, 2026 19:57
111a167 to
ac91f6b
Compare
Phase 5b, PR 5b.2 of the reputation portability plan (#674). A new Import reputation screen (/import_reputation): - "Get it from lnp2pBot" opens t.me/lnp2pbot?start=rep_<identity>, the identity as 43 base64url characters (Telegram caps the parameter). - The user pastes what the bot sent, or finds an attestation exported from another Mostro already filled in; the Rust core verifies it and the screen shows its figures before importing them into the node. - Refusals, local or from the node, are localized (reputation_errors.dart), in every ARB. ReputationApi wraps the bridge calls so the screen is tested without the Rust core. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrKanPgEfEBiEo3snHz4WZ
The Design guide check refused the theme-default Material buttons and the paste field's v1 decoration (DS-CMP-17, DS-CMP-19). The buttons now come from reputation_controls.dart, styled like the NWC screen's (lime primary and outlined secondary, radius 16), and the field sets its enabled and focused borders and filled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrKanPgEfEBiEo3snHz4WZ
check_reputation_import runs every check import_reputation makes before it asks the node (the node imports, privacy mode is off, the attestation verifies, names the identity and comes from a trusted issuer) without sending anything, so the import screen can refuse on Check instead of at the irreversible last step. Both share one helper; import_reputation's behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016jssAfzZj1LeWCnx7MHu2d
…e up front - Editing the text after Check withdraws the checked attestation (and an answer to a text that changed meanwhile is dropped), so Import only ever sends the attestation in the field. - Check runs check_reputation_import: an untrusted issuer or an attestation for another identity is refused before the user commits to the import. - A node that does not import, or full privacy mode, is said up front, and the bot button, the field and Check are disabled: the identity never goes to Telegram for an import that would be refused. - A link that could not be opened says so, with what to do by hand; the launcher sits behind ReputationApi so tests reach it. - The rating is formatted for the locale (DS-L10N-3), and the figures use ICU plurals in all six languages. The new keys carry descriptions. - Tests: pending prefill, stale state, local refusal, unsupported node, privacy mode, failed launch, locale, singular, and DS-A11Y-4 (German, 320 dp, 2× text, both themes). - The reputation automation ids get their own section. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016jssAfzZj1LeWCnx7MHu2d
grunch
force-pushed
the
feat/reputation-import-ui
branch
from
October 14, 2026 20:31
0a49c13 to
8844a09
Compare
Member
Author
|
@Catrya, following up on this review. The code findings are addressed at 8844a094, including fixes already present in commits after the reviewed Disposition of the blocking findings:
The non-blocking code findings are also addressed: launch failures and missing/invalid identity produce localized feedback; ratings, counts and days use locale formatting with ICU plurals; the complete import flow is documented in the living contracts; privacy is guarded within this screen; pending-prefill, stale-state and German 320 dp / 2× text tests are present; and the unrelated Validation: 30 focused reputation tests passed on the final changes; Rust tests passed with 1,280 passed and 30 ignored. All five jobs in the CI run for this head passed: Rust, Flutter, design guide, web build/smoke test, and iOS build without codesigning. Please re-review the code changes when convenient. Item 4 remains open; I am not claiming the live integration has been verified. |
This was referenced Oct 15, 2026
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
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.
Linked issue
Part of #674, phase 5b.2. The core dependency #675 is merged; this branch is now rebased directly on
main.Type
feat
What changes and why
Add the Import reputation screen for reputation earned on lnp2pBot or another Mostro. Users see their complete identity npub before confirming the binding in the bot, paste or prefill an attestation, check its signature/destination/issuer locally, inspect the figures, and explicitly import it into the active node.
The screen waits for privacy and identity checks before enabling actions. The Telegram handoff rechecks privacy, identity and node support; leaving the route cancels it, and a changed identity must be reviewed before retrying. Editing withdraws a checked attestation, and the input stays fixed while an import is in flight. Launch failures, missing identities and read failures have localized feedback. Counts, ratings and plurals follow the locale; screen readers receive result announcements.
Affected flow
/import_reputation, within the persistent home shell. The Settings entry remains part of the subsequent UI PR; this PR adds the route and screen.check_reputation_importshares the import's local checks;reputation_identity_npubencodes the displayed public key in Rust.reputation.mdnow document the screen's inputs, gates, side effects and failures.Blast radius
The existing export/import transport, identity publication guards and identity-scoped pending slot from #675 are preserved. No storage schema, wire format or dependency change is introduced. The pending attestation is read for prefill; import still repeats local checks and the daemon has the final decision.
The route follows main's navigation structure. All existing ARB values are preserved, with 24 added strings in every locale. FRB bindings and localizations were regenerated from source. The six
reputation.import.*automation IDs include the displayed identity readout; the automation contract is updated. Existing trade, wallet and notification flows are unchanged by the feature.Platforms tested
Linux host: Rust tests, clippy, native library build, Flutter analyzer and widget/unit suites. Rust also compile-checked for
wasm32-unknown-unknown. No Android/iOS/device run or web browser smoke test was performed in this review.Manual testing
Setup
No live app-to-Telegram-to-daemon import was run. The checks below are a pending manual procedure, not reported results. A run requires a compatible Mostro node trusting the source issuer and an eligible test lnp2pBot account. The screen currently has a route but no Settings entry in this PR.
Steps
Expected: the keys match; the bot provides an attestation after confirmation.
Expected: the checked figures and Import action disappear until the new text is checked. The older PR revision incorrectly kept the previous attestation; this screen does not exist on main.
Expected: the daemon confirms the import; an error or timeout is never shown as success.
Expected: the screen explains the gate and cannot open Telegram or import. Restore supported settings and Retry to reload the gates.
Expected: its pending JSON prefills and is locally checked; a successful matching import clears the pending slot.
Not covered
Live account binding/import, upgrade/restart on a device, actual Telegram handler behavior, Android/iOS, and web runtime behavior. No external account was bound during the review. Maintainer exemption applies; the earlier review's request for live integration evidence remains a validation limitation.
Screenshots
Visible feature. No device screenshots or recording were captured, and no manual UI results are claimed. Ten automated fixture-render tests passed; before/after renders were inspected locally in English/Spanish and both themes, plus German at 2x. They use a fake API and do not establish live integration behavior. Automated layout coverage exercises the screen and full-length npub in both themes at 320 dp, 2x text, German, with bundled fonts. Relevant rules: DS-CMP-3/4, DS-TYP-2/3, DS-L10N-1/2/3, DS-A11Y-2/4. No new color token.
Automated tests
cargo clippy --locked -- -D warnings,cargo check --locked --target wasm32-unknown-unknown, andcargo build --lockedpassed. Includes npub round-trip/invalid-input coverage and local import checks.flutter analyze: no issues. Design check: no breaks. Changed Dart and Rust files pass formatting;git diff --checkis clean.cargo fmt --checkreports existing formatting differences in 53 files, none changed by this PR. Unrelated files were left intact.Checklist
cargo fmt --checkpasses (pre-existing differences noted above); Rust tests/clippy/build passflutter analyzepassesAI assistance
The original implementation used Claude Code. Codex reviewed all PR feedback and the code, resolved rebase conflicts, added regression tests, fixed the remaining issues, and ran the checks reported above.