Repository navigation
Conversation
Phase 5b, PR 5b.1 of the reputation portability plan (#674). Points mostro-core at its reputation branch until the release is published. - mostro::reputation_support keeps, per node, what its info event advertises: reputation_issuer and reputation_import_issuers. - api::reputation_transfer: export_reputation, import_reputation, sign_reputation_rebind, parse_reputation_attestation, get_reputation_support and get_pending_reputation_attestation. Each request is sent only to a node that advertises it, never in privacy mode, from a fresh trade key with the identity proof, and correlated by its nonce through ask_daemon_with, which now takes the reply window and log category. Errors are markers Dart can localize. - The exported attestation is kept until imported, under an identity-scoped settings key wiped with the identity on both stores. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrKanPgEfEBiEo3snHz4WZ
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
Phase 5b, PR 5b.3 of the reputation portability plan (#674). Settings gains a Reputation row (Network group) leading to a screen that: - exports the user's reputation from the active node, after a dialog showing the identity as an npub and saying the export binds the account to it, then shows the attestation to copy; - opens the import screen (5b.2); - signs a rebind authorisation for a new identity (npub or hex; the core now parses either), to copy to the new device or into lnp2pBot; - explains that privacy mode keeps both directions off. Strings in every ARB; automation ids settings.reputation and reputation.*. The settings goldens change with the new row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrKanPgEfEBiEo3snHz4WZ
|
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 54 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 507c461d0f
ℹ️ 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".
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
Same fix as the import screen: the export, import and rebind buttons use reputation_controls.dart instead of theme-default Material buttons (DS-CMP-17), the new-identity field sets its borders and filled (DS-CMP-19), and the card title takes a size on the type scale (DS-TYP-4). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UrKanPgEfEBiEo3snHz4WZ
Catrya
left a comment
There was a problem hiding this comment.
Review evidence
Decision: Request changes
Tested commit: 88ee7ad3 (PR head). I reviewed the PR's own diff against #676's head (2615478f..88ee7ad3). I also merged the whole stack 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#1027.
- App source: Pull request #677, with #675 and #676 below it, merged onto current
main.
Commands and tests executed:
git merge --no-ff --no-commit origin/feat/reputation-settings-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 probe 1:
hexToNpubagainst a reference bech32 encoder, on 202 keys (all-zero, all-0xff, 200 random), in lower and upper case. - Throwaway probe 2:
ReputationScreenat 320 dp wide, 2× text, German, privacy mode on and off. It scrolls the wholeListView, checks for overflow and records which buttons are enabled. - Read the PR's own diff in full, plus
rust/src/api/reputation_transfer.rsandreputation_api.dartfrom the stack. - Compared with v1: MostroP2P/mobile#745 (open).
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 (8 files).flutter test: 2145 passed, including the PR's tests and the regeneratedsettings_10agoldens.- Probe 1: 202 of 202 match.
- Probe 2: no overflow anywhere on the screen, and privacy mode disables the export, import and sign buttons.
- 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.
- Merge with
- Skipped: 4 in
flutter test, also skipped onmain. Web smoke test not run on the merge. - Flaky: None observed
Manual observations:
- The export dialog shows the same key the export signs with. Both come from the identity key:
identity_api.getIdentity().publicKeyandget_active_keys(). Rust also refuses in privacy mode (identity_keys()), so the UI is not the only gate. - Nothing in the app ever passes
rebindtoexport_reputation._export()callsapi.export()with no arguments, and the import screen does not accept an authorization. - The PR's own branch carries
145080b0, which is the same patch as #676's2615478f. The branch is not rebased on #676's current head. rust/Cargo.tomlon the stack still pinsmostro-coretobranch = "feat/reputation-merge", which no longer exists.mainis on 0.17.1, and the stack builds and passes against it.- The PR is a near-literal port of mobile#745. That PR has the same rebind gap and also signs without confirmation; it encodes the npub with v1's existing
NostrUtils. - Codex's four comments on this PR (2026-10-03) have no reply and no change. The last push was 2026-10-03 16:29.
Not tested or limitations:
- No export, import or rebind from this app against a daemon. No local daemon carries mostro#1027, and the PR's own body says "Not run on a device yet".
- No web build or smoke test on the merge.
- No device or screen check. The PR has no screenshots.
- The stack was tested with
mostro-core0.17.1 frommain, not with the PR's pin.
Unresolved findings:
Blocking:
- The Mostro-to-Mostro move to a new identity is a dead end.
- The screen signs a rebind authorization and tells the user to "use it from the new identity", but no screen consumes it:
_export()never passesrebind, and import does not take one. - From the new identity the node refuses with
ReputationBoundToOtherIdentity. Its message asks the user to authorize the move from the bound identity, which they already did, so the user loops. - Only the lnp2pBot path (pasting into the bot chat) works.
- Fix: add a field for the authorization on export from the new identity, or scope the rebind card explicitly to lnp2pBot. Codex P1, open.
- The screen signs a rebind authorization and tells the user to "use it from the new identity", but no screen consumes it:
- NIP-19 encoding is hand-written in Dart (
lib/features/reputation/npub.dart).- The architecture rule puts Nostr protocol logic in Rust, and
nostr-sdkalready providesto_bech32(). - The encoder is correct (202/202), but it is a second protocol implementation, and it sits in the dialog that confirms the binding.
- Fix: expose the npub from Rust, for example as an
npubfield onIdentityInfo, and deletenpub.dart. Codex P1, open.
- The architecture rule puts Nostr protocol logic in Rust, and
- Signing the rebind authorization asks for no confirmation.
- The authorization lets whatever key is typed export this reputation and take the binding. That is the more dangerous step, more so than the export the PR asks reviewers to examine.
- A user persuaded to paste an attacker's npub loses their reputation.
- Fix: show the target npub and the consequence in a
MostroDialogbefore signing, as export does.
- No end-to-end run.
- The Manual testing steps were written but never executed: "Not run on a device yet", "Screenshots: Not taken yet".
- Maintainers are exempt from the contribution bar. But this is the first PR in the stack where the flow can be run in the app, #675 was not run end to end either, and export creates a binding on the node that does not undo itself.
- Needed: one real export and one real import against a daemon with mostro#1027.
- The stack cannot merge as it stands.
Non-blocking:
- Capabilities never refresh.
support()is read withref.readinbuild, and nothing rebuilds the screen reputation." until it is reopened. Codex P2, open. - No living contract. There is still no contract for reputation portability in
specs/004-mostro-p2p-client/contracts/, and none records thatsign_reputation_rebindnow accepts an npub. Same finding as on #675. - Misleading error text.
reputationInvalidIdentitysays "That is not a valid npub." although hex is accepted, and it is the fallback for any error from signing the rebind. - Rebind output has no automation id.
_Copyable(text: _rebind!)carries none, despite "all attached" in the body. _Copyable:- the
IconButtonhas no tooltip or semantic label; - there is no "Copied" feedback, and
Clipboard.setDatais not awaited; - the value is cut to 2 lines and cannot be selected.
- the
- Tests:
- the rebind test's validation lives in the fake (
startsWith('npub1')), so it does not exercise the real check; PublicKey::parseinsign_reputation_rebindhas no Rust test for hex, npub and garbage input;- "privacy mode turns both directions off" asserts only the export button;
- the DS-A11Y-4 test (320 dp, 2× text, German) is missing. The behaviour already holds (probe 2), but no test pins it.
- the rebind test's validation lives in the fake (
- Row placement: the Settings row sits in the Network group, and reputation is not a network setting.
0a49c13 to
8844a09
Compare
|
Superseded by #827. That PR carries this branch's two commits rebased onto @Catrya, here is where each of your findings landed: Blocking
Non-blocking
|
Linked issue
Part of #674 (phase 5b, PR 5b.3), the last app PR of the plan. It is stacked on #676 (5b.2).
Type
feat
What changes and why
Settings gains a Reputation row in the Network group, leading to the new
ReputationScreen(AppRoute.reputation):reputation_issuer(get_reputation_support); otherwise the screen says the node does not export.MostroDialogthat shows the user's identity as an npub (npub.dart, NIP-19 bech32) and explains that the export binds this node's account to it. This is the confirmation the protocol requires before a first binding.sign_reputation_rebind, which now parses either form withPublicKey::parse;Affected flow
lib/features/reputation/screens/reputation_screen.dartandnpub.dart.SettingsRowinsettings_screen.dart, with idsettings.reputation.gen-l10ncommitted, nothing untranslated).reputation.*ids plus the settings row, all attached.sign_reputation_rebindaccepts an npub. The change is body-only, so the generated code is unchanged.Blast radius
10a_settings; I triggeredupdate-goldens.ymlon this branch to regenerate them, asdocs/golden-tests.mdprescribes.Platforms tested
Linux (widget tests).
Manual testing
Setup
A build from this branch against a node with
[reputation_export](MostroP2P/mostro#1033–#1035), with an identity that has 10 completed and 5 rated trades there.Steps
Expected: the Export card is shown.
Expected: the dialog shows your npub. Confirming shows "Your reputation is ready".
Expected: "Your reputation was imported".
Expected: both directions are off, with the notice.
Not covered
Not run on a device yet.
Screenshots
Not taken yet. The settings goldens are regenerated in CI.
Automated tests
npub_test.dart: the NIP-19 example key encodes to its published npub.reputation_screen_test.dart:modal_guard_testpasses: the dialog goes throughshowMostroDialog.Full suite: the only failures are the settings goldens this PR changes, being regenerated in CI, and the known
attachment_bubblesgoldens.Checklist
flutter analyzeandflutter testpass locally (goldens aside, see above)gen-l10noutput committedAI assistance
Written with Claude Code. Look hardest at the export confirmation, the step that binds the account.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UrKanPgEfEBiEo3snHz4WZ