feat(us1): phase 3 — first launch & identity setup - #52
Merged
Conversation
Rust crypto layer: - keys.rs: BIP-39 mnemonic generation + BIP-32 derivation at m/44'/1237'/38383'/0/N for identity and trade keys - ecdh.rs: NIP-44 v2 encrypt/decrypt + k256 diffie_hellman for raw shared-secret derivation (file-attachment encryption) - nym.rs: deterministic pseudonym from SHA-256(pubkey) — adjective-noun + icon index 0–36 + hue 0–359 Identity API (rust/src/api/identity.rs): - create_identity, load_identity_from_mnemonic, import_from_mnemonic, import_from_nsec, get/delete identity, derive/get trade key, get_nym_identity, export_encrypted_backup - In-memory OnceLock<RwLock<Option<IdentityState>>>; mnemonic never persisted by Rust Flutter US1: - WalkthroughScreen: 6 slides via introduction_screen, custom dots (pill 16×8 active, circle 8×8 inactive), Done/Skip navigation - HighlightConfig: per-slide regex patterns for green semibold terms in EN/ES/IT/FR/DE - firstRunProvider + backupReminderProvider via shared_preferences - GoRouter redirect: firstRunComplete=false → /walkthrough; routerContainer wired in MostroApp for context-free provider reads
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
WalkthroughSets up router redirect to read Riverpod first-run state via a global Changes
Sequence DiagramsequenceDiagram
participant App as MostroApp
participant Router as AppRouter
participant Provider as firstRunProvider
participant Persist as SharedPreferences
participant Screen as WalkthroughScreen
App->>Router: set global routerContainer via ProviderScope
Router->>Provider: read firstRunProvider (async)
Provider->>Persist: load `_kFirstRunComplete` (default false)
Persist-->>Provider: return persisted value
Provider-->>Router: AsyncValue<bool> (firstRunComplete)
alt firstRunComplete == false
Router->>Screen: redirect to WalkthroughScreen
Screen->>Provider: markFirstRunComplete()
Provider->>Persist: save firstRunComplete = true
Provider-->>Screen: state updated
Screen->>Router: navigate to AppRoute.home
else
Router->>App: proceed with requested route
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
rust/src/crypto/ecdh.rs (1)
88-97: Document the even-Y parity assumption.The function always uses
0x02prefix (even Y), which is correct per BIP-340/Nostr convention for x-only pubkeys. A brief comment would clarify this design decision for future maintainers.📝 Add clarifying comment
/// Convert a 32-byte x-only pubkey to a 33-byte compressed SEC1 point. +/// Per BIP-340 and Nostr convention, x-only keys assume even Y parity. fn compress_from_xonly(xonly: &[u8]) -> Result<Vec<u8>> { if xonly.len() != 32 { return Err(anyhow!("expected 32-byte x-only pubkey, got {}", xonly.len())); } let mut out = Vec::with_capacity(33); - out.push(0x02); // assume even Y (matches Nostr convention) + out.push(0x02); // Even Y parity per BIP-340/Nostr x-only convention out.extend_from_slice(xonly); Ok(out) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/crypto/ecdh.rs` around lines 88 - 97, The compress_from_xonly function currently always prepends 0x02 (even Y) when converting a 32-byte x-only pubkey to a 33-byte compressed SEC1 point; add a brief clarifying comment above/composed into compress_from_xonly explaining that this hardcoded 0x02 implements the BIP-340/Nostr convention of treating the x-only pubkey as having an even Y coordinate, so maintainers know the parity assumption is intentional and not a bug.rust/src/crypto/nym.rs (1)
72-97: Consider adding a fixed test vector for regression testing.The current tests use randomly generated keys, which validates properties but doesn't catch accidental changes to the derivation algorithm. A fixed pubkey with expected output would catch regressions.
🧪 Proposed test with fixed vector
+ #[test] + fn known_pubkey_produces_known_nym() { + // Fixed test vector to catch algorithm changes + let hex = "82341f882b6eabcd2ba7f1ef90aad961cf074af15b9ef44a09f9d2a8fbfbe6a2"; + let nym = get_nym_identity(hex).unwrap(); + // Update these expected values once after initial implementation + assert!(!nym.pseudonym.is_empty()); + assert!(nym.icon_index <= 36); + assert!(nym.color_hue <= 359); + // Optionally pin exact values: + // assert_eq!(nym.pseudonym, "expected-nym"); + // assert_eq!(nym.icon_index, expected_index); + // assert_eq!(nym.color_hue, expected_hue); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/crypto/nym.rs` around lines 72 - 97, Add a deterministic regression test using a fixed public key hex and its expected NymIdentity to catch algorithm regressions: create a new test (or extend existing same_pubkey_same_identity) that calls get_nym_identity with a hard-coded public key hex string and asserts equality against the known expected pseudonym, icon_index, and color_hue values (referencing get_nym_identity, same_pubkey_same_identity, icon_index_in_range, color_hue_in_range to locate test area); if get_nym_identity returns a Result, unwrap or handle the error consistently and lock the expected values into the test so any future change in derivation fails the test.lib/features/walkthrough/providers/first_run_provider.dart (1)
31-41: Consider refactoring to avoid passingWidgetRefinto the StateNotifier.Passing
WidgetReftomarkFirstRunCompletecouples this notifier to widget-layer concerns. A cleaner pattern would be to injectbackupReminderProvider.notifiervia the provider'srefat construction time, or have the caller invoke both notifiers separately.This works correctly as-is, but could be improved for testability and separation of concerns.
♻️ Suggested refactor to remove WidgetRef dependency
final firstRunProvider = StateNotifierProvider<FirstRunNotifier, AsyncValue<bool>>( - (ref) => FirstRunNotifier(), + (ref) => FirstRunNotifier(ref), ); class FirstRunNotifier extends StateNotifier<AsyncValue<bool>> { - FirstRunNotifier() : super(const AsyncValue.loading()) { + FirstRunNotifier(this._ref) : super(const AsyncValue.loading()) { _load(); } + final Ref _ref; + // ... _load() unchanged ... /// Mark the walkthrough as completed. Called by both "Done" and "Skip". /// /// Also activates the backup reminder (red dot on notification bell). - Future<void> markFirstRunComplete(WidgetRef ref) async { + Future<void> markFirstRunComplete() async { final prefs = await SharedPreferences.getInstance(); await prefs.setBool(_kFirstRunComplete, true); state = const AsyncValue.data(true); // Activate persistent backup reminder notification. - ref.read(backupReminderProvider.notifier).showBackupReminder(); + _ref.read(backupReminderProvider.notifier).showBackupReminder(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/walkthrough/providers/first_run_provider.dart` around lines 31 - 41, The markFirstRunComplete method currently accepts a WidgetRef which couples the StateNotifier to widget-layer APIs; refactor by removing the WidgetRef parameter from markFirstRunComplete and instead inject the backup reminder notifier at construction time (e.g., accept a BackupReminderNotifier or AutoDisposeNotifierBase instance in the FirstRunProvider/StateNotifier constructor obtained from the provider ref when creating the notifier), or alternatively leave markFirstRunComplete to only update SharedPreferences and state and require callers to separately call backupReminderProvider.notifier.showBackupReminder(); update all call sites that used markFirstRunComplete(ref) to call the new signature or to call the backup reminder separately and ensure tests construct the notifier with a mock/injected backup reminder instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/api/identity.rs`:
- Around line 183-197: The code in derive_trade_key currently increments
state.identity_info.trade_key_index before calling key_ops::derive_trade_key,
which can leave the stored index advanced if derivation fails; change the flow
in derive_trade_key to call key_ops::derive_trade_key(&state.mnemonic_words,
candidate_index) first (using a local candidate_index computed as
state.identity_info.trade_key_index + 1), only increment and persist
state.identity_info.trade_key_index after a successful derivation, and then
construct and return the TradeKeyInfo from the derived keys; reference
derive_trade_key, state.identity_info.trade_key_index,
key_ops::derive_trade_key, and TradeKeyInfo to locate the changes.
- Around line 251-261: The code uses a fixed all-zero nonce (variable nonce)
with ChaCha20Poly1305 which is unsafe; change the encryption to generate a
cryptographically random 12-byte nonce (e.g., via rand::rngs::OsRng) per
encryption, use that nonce with ChaCha20Poly1305::encrypt, and persist/serialize
the nonce along with the ciphertext (prepend or include in envelope) so decrypt
can use the same nonce; update dependencies to include rand and ensure the code
paths around key_bytes, cipher, nonce, and encrypt handle the nonce being random
and stored with the ciphertext.
- Around line 112-119: The IdentityInfo creation currently sets created_at to
unix_now() which overwrites the original Flutter-persisted creation time; update
the code in the function that constructs IdentityInfo (e.g.,
load_identity_from_mnemonic / the block that builds IdentityInfo using
public_key) to accept and use a passed-in created_at timestamp from Flutter
instead of always calling unix_now(), and only fall back to unix_now() when the
incoming created_at is absent or invalid so original creation time is preserved.
In `@rust/src/crypto/ecdh.rs`:
- Around line 15-39: The function derive_shared_key currently returns the NIP-04
style SHA256(ECDH_x) (via ecdh_sha256) while also deriving a NIP-44
ConversationKey and discarding it; fix by either (A) renaming the function and
its docs to reflect NIP-04 semantics (e.g., derive_nip04_shared_key or
derive_ecdh_sha256) and keep returning ecdh_sha256, or (B) actually return the
NIP-44 v2 conversation key bytes: use
nostr_sdk::nips::nip44::v2::ConversationKey::derive (as already called) and then
materialize its raw key bytes by using a stable approach available in this SDK
version (e.g., if raw extraction isn’t exposed, perform a deterministic
encrypt/decrypt round-trip or call any provided export method) and replace the
ecdh_sha256 return with those HKDF-derived bytes; update function name/docs
accordingly (refer to derive_shared_key, ConversationKey, and ecdh_sha256 to
locate edits).
---
Nitpick comments:
In `@lib/features/walkthrough/providers/first_run_provider.dart`:
- Around line 31-41: The markFirstRunComplete method currently accepts a
WidgetRef which couples the StateNotifier to widget-layer APIs; refactor by
removing the WidgetRef parameter from markFirstRunComplete and instead inject
the backup reminder notifier at construction time (e.g., accept a
BackupReminderNotifier or AutoDisposeNotifierBase instance in the
FirstRunProvider/StateNotifier constructor obtained from the provider ref when
creating the notifier), or alternatively leave markFirstRunComplete to only
update SharedPreferences and state and require callers to separately call
backupReminderProvider.notifier.showBackupReminder(); update all call sites that
used markFirstRunComplete(ref) to call the new signature or to call the backup
reminder separately and ensure tests construct the notifier with a mock/injected
backup reminder instance.
In `@rust/src/crypto/ecdh.rs`:
- Around line 88-97: The compress_from_xonly function currently always prepends
0x02 (even Y) when converting a 32-byte x-only pubkey to a 33-byte compressed
SEC1 point; add a brief clarifying comment above/composed into
compress_from_xonly explaining that this hardcoded 0x02 implements the
BIP-340/Nostr convention of treating the x-only pubkey as having an even Y
coordinate, so maintainers know the parity assumption is intentional and not a
bug.
In `@rust/src/crypto/nym.rs`:
- Around line 72-97: Add a deterministic regression test using a fixed public
key hex and its expected NymIdentity to catch algorithm regressions: create a
new test (or extend existing same_pubkey_same_identity) that calls
get_nym_identity with a hard-coded public key hex string and asserts equality
against the known expected pseudonym, icon_index, and color_hue values
(referencing get_nym_identity, same_pubkey_same_identity, icon_index_in_range,
color_hue_in_range to locate test area); if get_nym_identity returns a Result,
unwrap or handle the error consistently and lock the expected values into the
test so any future change in derivation fails the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15a91bb9-20fe-4e9d-8597-514d00287990
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
lib/core/app.dartlib/core/app_routes.dartlib/features/walkthrough/providers/first_run_provider.dartlib/features/walkthrough/screens/walkthrough_screen.dartlib/features/walkthrough/utils/highlight_config.dartrust/Cargo.tomlrust/src/api/identity.rsrust/src/api/mod.rsrust/src/crypto/ecdh.rsrust/src/crypto/keys.rsrust/src/crypto/mod.rsrust/src/crypto/nym.rsrust/src/nostr/order_events.rsspecs/004-mostro-p2p-client/tasks.md
identity.rs: - derive_trade_key: compute candidate_index before calling derivation; only persist incremented trade_key_index on success to avoid advancing the counter on a failed key derivation - load_identity_from_mnemonic: accept created_at: Option<i64> so the original Flutter-persisted creation timestamp is preserved on reload; falls back to unix_now() when absent or zero - export_encrypted_backup: replace fixed zero nonce with OsRng random 12-byte nonce per call; prepend nonce to ciphertext before base64 encoding (format: [12-byte nonce][ciphertext+tag]); add rand dep ecdh.rs: - rename derive_shared_key → derive_nip04_shared_key and rewrite docs to accurately reflect NIP-04 semantics (SHA-256 of ECDH x-coord); remove dead ConversationKey derivation that was silently discarded - compress_from_xonly: expand comment to cite BIP-340/Nostr as source of the unconditional even-Y assumption nym.rs: - add known_pubkey_regression test with secp256k1 generator pubkey; asserts pseudonym="tall-crane", icon_index=35, color_hue=249 to catch any future algorithm drift first_run_provider.dart: - remove WidgetRef parameter from markFirstRunComplete to decouple StateNotifier from widget-layer APIs; call site activates backupReminderProvider separately
This was referenced Mar 29, 2026
Merged
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
1 participant
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.
Rust crypto layer:
Identity API (rust/src/api/identity.rs):
Flutter US1:
Summary by CodeRabbit
New Features
Bug Fixes
Chores