Skip to content

feat: database crate#225

Merged
heilhead merged 2 commits into
mainfrom
feat/database-crate-rebased
Jul 8, 2025
Merged

feat: database crate#225
heilhead merged 2 commits into
mainfrom
feat/database-crate-rebased

Conversation

@heilhead

@heilhead heilhead commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

Description

This adds wcn_db crate for a standalone database server binary.

How Has This Been Tested?

Existing tests and some new ones.

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@heilhead heilhead force-pushed the feat/database-crate-rebased branch from 1d2388b to d0c9c70 Compare July 5, 2025 13:06
@heilhead heilhead marked this pull request as ready for review July 5, 2025 13:08
@heilhead heilhead requested review from nopestack and xDarksome and removed request for xDarksome July 5, 2025 13:08
@heilhead heilhead force-pushed the feat/database-crate-rebased branch from 1c9e1be to e65aa03 Compare July 5, 2025 13:12
@heilhead heilhead requested a review from Copilot July 5, 2025 13:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a new wcn_db crate to provide a standalone database server binary and migrates existing code to use the new wcn_rocks and wcn_logging crates.
Key changes:

  • Swapped out relay_rocks for wcn_rocks across node and storage layers, and updated TTL methods from exp/setexp/hexp/hsetexp to get_exp/set_exp/hget_exp/hset_exp.
  • Renamed error payload fields from details to message with with_message builder methods.
  • Introduced convenience impls (from_bytes/as_bytes, from_unix_timestamp_*, to_unix_timestamp_*) for Namespace, RecordExpiration, and RecordVersion.

Reviewed Changes

Copilot reviewed 43 out of 44 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/rpc/src/quic/client.rs Bug in named-format usage for format!.
crates/storage_api2/src/rpc/mod.rs Renamed RPC error field detailsmessage.
crates/rocks/src/db/types/common/iterators.rs Renamed has_morehas_next in scan result.
crates/wcn/src/commands/node/{stop.rs,start.rs,mod.rs} Updated logger import to wcn_logging.
crates/rocks/src/db/types/{string.rs,map.rs,mod.rs} Unified Record and MapRecord types and renamed methods.
Comments suppressed due to low confidence (3)

crates/storage_api2/src/rpc/mod.rs:159

  • Changing the serialized field name from details to message is a breaking change for RPC clients. Consider adding a #[serde(rename = "details")] or #[serde(alias = "details")] on message to maintain backward compatibility.
    message: Option<String>,

crates/rocks/src/db/types/common/iterators.rs:48

  • Renaming the scan result field has_more to has_next changes the public API. Ensure consumers are updated and consider deprecating the old field or documenting the rename.
    pub has_next: bool,

crates/rpc/src/quic/client.rs:198

  • Using named braces inside format! without supplying named arguments will fail to compile. Either switch back to positional formatting ("... {} ... {}", peer_id, actual_peer_id) or use named parameters (format!("... {peer_id} ... {actual_peer_id}", peer_id=peer_id, actual_peer_id=actual_peer_id)).
                    "Invalid peer ID: expected: {peer_id} actual: {actual_peer_id}"

Comment thread crates/db/src/config.rs

Ok(Self {
keypair: raw.keypair,
db_port: raw.db_port.unwrap_or(3010),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider making these default values constants instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be inconsistent with other crates. If we decide to change the way we initialize these configs, we should update all of the binary crates that initialize this way.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can be done progressively though, minor nit though

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already discussed that multiple times @nopestack and agreed that if the var is being used in one place only it's fine to inline it

@heilhead heilhead force-pushed the feat/database-crate-rebased branch from 3dd2585 to fd72ff5 Compare July 7, 2025 07:00
Comment thread crates/db/Cargo.toml Outdated
@heilhead heilhead force-pushed the feat/database-crate-rebased branch 2 times, most recently from 6d8d954 to d255003 Compare July 7, 2025 07:39
Comment thread Cargo.toml
Comment thread crates/db/src/lib.rs Outdated
Comment thread crates/db/src/storage.rs
Comment thread crates/logging/Cargo.toml
@heilhead heilhead force-pushed the feat/database-crate-rebased branch from dd757f7 to ef371dc Compare July 8, 2025 09:42
@heilhead heilhead merged commit 044270d into main Jul 8, 2025
10 of 11 checks passed
@heilhead heilhead deleted the feat/database-crate-rebased branch July 8, 2025 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants