Refactor/setup account #310
Conversation
…tion from publish_relay_list_for_account
|
Caution Review failedThe pull request is closed. WalkthroughAdds a default Justfile recipe; refactors account creation/login into private helper steps without public API changes; introduces a pubkey-centric relay-publishing helper, updates publishing logic and tag kinds, and adds tests for relay-list event construction. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Whitenoise
participant SecretStore
participant DB
participant Relays
participant Subscriptions
User->>Whitenoise: create_identity(petname, ...)
activate Whitenoise
Whitenoise->>SecretStore: store private key
Whitenoise->>Whitenoise: setup_relays_for_new_account()
Whitenoise->>DB: persist_account()
Whitenoise->>Relays: connect and publish defaults
Whitenoise->>Whitenoise: setup_key_package()
Whitenoise->>Subscriptions: setup subscriptions
Whitenoise->>Whitenoise: setup_metadata(petname)
deactivate Whitenoise
sequenceDiagram
participant User
participant Whitenoise
participant SecretStore
participant DB
participant Relays
participant Subscriptions
participant Fetcher
User->>Whitenoise: login(credentials)
activate Whitenoise
Whitenoise->>SecretStore: store private key
Whitenoise->>Whitenoise: setup_relays_for_existing_account()
Whitenoise->>DB: persist_account()
Whitenoise->>Relays: connect
Whitenoise->>Whitenoise: setup_key_package()
Whitenoise->>Subscriptions: setup subscriptions
Whitenoise-->>Fetcher: trigger background data fetch
deactivate Whitenoise
sequenceDiagram
participant Account
participant Whitenoise
participant Signer
participant TargetRelays
Account->>Whitenoise: publish_relay_list_for_account(type, target_relays?)
Whitenoise->>Whitenoise: compute relay_list and relays_to_use
Whitenoise->>Signer: build+sign event (tags vary by RelayType)
Whitenoise->>TargetRelays: publish event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/whitenoise/accounts/mod.rs (1)
600-617: Persist: good cleanup on failure; minor consistency nits
- Cleanup of stored private key on DB save failure is appreciated.
- Consider standardizing tracing targets (they vary between
setup_accountandpersist_and_activate_account) for easier log filtering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
justfile(1 hunks)src/whitenoise/accounts/mod.rs(3 hunks)src/whitenoise/accounts/relays.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/whitenoise/accounts/mod.rs (2)
src/whitenoise/utils.rs (1)
capitalize_first_letter(128-134)src/whitenoise/secrets_store.rs (1)
new(44-48)
src/whitenoise/accounts/relays.rs (1)
src/whitenoise/accounts/mod.rs (1)
new(183-200)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (macos-latest, native)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (5)
justfile (1)
5-7: LGTM: Default recipe lists available commandsThis is a sensible default and non-invasive. No issues.
src/whitenoise/accounts/relays.rs (3)
280-309: Helper correctness: tag mapping and signer-based publishThe new helper cleanly maps:
- Nostr -> "r" tags (Tag::reference), kind 10002
- Inbox/KeyPackage -> "relay" tags (Tag::custom(TagKind::Relay)), kinds 10050/10051
Using
publish_event_builder_with_signerwith the per-pubkey signer is correct, and passingtarget_relaysby value avoids unnecessary cloning. Looks good.
950-1024: Tests for Nostr relay-list publishing are spot onThe test asserts correct kind, tag types (only 'r' tags), counts, and empty content. This validates observable behavior well. Nice.
1026-1110: Tests for Inbox/KeyPackage relay-list publishing are comprehensiveLooping over both kinds, asserting 'relay' tags only, and verifying contents keeps coverage DRY and focused. Solid.
src/whitenoise/accounts/mod.rs (1)
258-269: No action needed: relay publishing already ensures connectionsThe
publish_event_builder_with_signermethod callsensure_relays_connected(relays.clone()).awaitbefore sending any events, so relays are connected on-demand prior to publishing. You can safely ignore the suggested refactor for pre-connecting insetup_relays_for_new_account.Likely an incorrect or invalid review comment.
| async fn setup_metadata(&self, account: &Account) -> Result<()> { | ||
| let petname = petname::petname(2, " ") | ||
| .unwrap_or_else(|| "Anonymous User".to_string()) | ||
| .split_whitespace() | ||
| .map(Whitenoise::capitalize_first_letter) | ||
| .collect::<Vec<_>>() | ||
| .join(" "); | ||
|
|
||
| let key_package_relays = if is_new_account { | ||
| Account::default_relays() | ||
| } else { | ||
| match self | ||
| .fetch_relays_from(nip65_relays.clone(), pubkey, RelayType::KeyPackage) | ||
| .await | ||
| { | ||
| Ok(relays) if !relays.is_empty() => relays, | ||
| _ => { | ||
| need_to_publish_key_package = true; | ||
| Account::default_relays() | ||
| } | ||
| } | ||
| let metadata = Metadata { | ||
| name: Some(petname.clone()), | ||
| display_name: Some(petname), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| // Step 3: Create account struct | ||
| let account = Account { | ||
| pubkey, | ||
| settings: AccountSettings::default(), | ||
| last_synced: Timestamp::zero(), | ||
| nip65_relays, | ||
| inbox_relays, | ||
| key_package_relays, | ||
| nostr_mls: Account::create_nostr_mls(pubkey, &self.config.data_dir)?, | ||
| }; | ||
| self.update_metadata(&metadata, account).await?; | ||
| tracing::debug!(target: "whitenoise::setup_metadata", "Created and published metadata with petname: {}", metadata.name.as_ref().unwrap_or(&"Unknown".to_string())); | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the capitalization helper is resolvable
Whitenoise::capitalize_first_letter suggests an associated method, but the provided snippet shows a free function whitenoise::utils::capitalize_first_letter(...). If the associated method doesn’t exist, this won’t compile. Either import and call the free function or add an associated wrapper.
Run this to confirm the helper exists as an associated function:
🏁 Script executed:
#!/bin/bash
# Find any associated method named `capitalize_first_letter` on Whitenoise
rg -n $'impl\\s+Whitenoise\\s*\\{[^}]*capitalize_first_letter' -A 2 -B 2
# Also find the free function
rg -n $'fn\\s+capitalize_first_letter\\s*\\('Length of output: 208
Fix capitalization helper reference
The code currently calls Whitenoise::capitalize_first_letter, but no such associated method exists. The helper is defined as a free function in src/whitenoise/utils.rs. You must either:
- Import and call the free function directly
use crate::utils::capitalize_first_letter; // … .map(capitalize_first_letter)
- Or add a wrapper in
impl Whitenoise { … }that delegates to the free function.
Locations to update:
- src/whitenoise/accounts/mod.rs (around line 585): replace
Whitenoise::capitalize_first_letterwith the importedcapitalize_first_letter
Example diff:
use petname;
+use crate::utils::capitalize_first_letter;
// …
- .map(Whitenoise::capitalize_first_letter)
+ .map(capitalize_first_letter)
// …🤖 Prompt for AI Agents
In src/whitenoise/accounts/mod.rs around lines 581 to 599, the code calls a
non-existent associated method Whitenoise::capitalize_first_letter; import the
free helper from the utils module and use it directly: add a use
crate::whitenoise::utils::capitalize_first_letter; (or an appropriate relative
path) at the top of the file and replace
.map(Whitenoise::capitalize_first_letter) with .map(capitalize_first_letter).
Ensure the import path matches the crate/module layout.
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
One small naming suggestion but all good.
src/whitenoise/accounts/mod.rs
Outdated
| self.background_fetch_account_data(&account).await?; | ||
| tracing::debug!(target: "whitenoise::setup_account", "Background data fetch triggered"); | ||
| } | ||
| fn create_base_account_with_stored_key(&self, keys: &Keys) -> Result<Account> { |
There was a problem hiding this comment.
maybe change the name of this to be create_base_account_with_key (the key isn't stored until this method is called)
There was a problem hiding this comment.
renamed to create_base_account_with_private_key
Removes setup_account and creates individual private functions for the required steps
jgmontoya
force-pushed
the
refactor/setup-account
branch
from
2f83882 to
90a65f6
Compare
Summary by CodeRabbit