docs: add missing crate and module-level documentation#769
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR comprehensively improves the pallas codebase by adding extensive Rustdoc comments across all crates, refactoring the umbrella crate's public API from alias re-exports to explicit modules, adding protocol termination message variants for graceful shutdown, moving multiplexer queue operations to AgentChannel, introducing new API methods, and exporting previously internal modules. ChangesDocumentation and API Surface Refinement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
pallas-network/src/miniprotocols/txmonitor/client.rs (1)
7-222:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing public method to send the
Donemessage.The client validates that
Message::Donecan be sent from theIdlestate (line 79), and the protocol now includes this variant (pallas-network/src/miniprotocols/txmonitor/protocol.rsline 64), but there's no public API to actually send it. Other miniprotocol clients in this PR (e.g.,peersharing::Client::send_done) expose this functionality.➕ Suggested method to add
/// Terminate the protocol. pub async fn done(&mut self) -> Result<(), Error> { let msg = Message::Done; self.send_message(&msg).await?; self.0 = State::Done; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pallas-network/src/miniprotocols/txmonitor/client.rs` around lines 7 - 222, Add a public async method named done(&mut self) -> Result<(), Error> to impl Client that constructs Message::Done, calls self.send_message(&msg).await? to perform the send (this will enforce agency/outbound checks), then sets self.0 = State::Done and returns Ok(()); place it alongside the other public API methods (e.g., after release or with other query methods). Ensure the method signature and state transition match the existing pattern used by send_acquire/release/query_* and that Message::Done is the variant validated by assert_outbound_state.pallas-network/src/miniprotocols/handshake/server.rs (1)
91-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn
InvalidInboundfor an unexpected client message here.This branch is handling a message received from the peer, so returning
InvalidOutboundmisclassifies the failure and makes debugging noisier downstream.Proposed fix
- _ => Err(Error::InvalidOutbound), + _ => Err(Error::InvalidInbound),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pallas-network/src/miniprotocols/handshake/server.rs` around lines 91 - 98, In receive_proposed_versions (method on the handshake server) the unexpected-message branch currently returns Error::InvalidOutbound but this is a message we received from the peer, so change that to return Error::InvalidInbound; update the match arm in receive_proposed_versions to construct and return Error::InvalidInbound instead of InvalidOutbound to correctly classify the error for downstream debugging and logs.pallas-network/src/facades.rs (2)
97-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExit the keep-alive server loop after protocol termination.
recv_keepalive_request()can move the server toDone, but this loop immediately callskeepalive_roundtrip()again. A clean shutdown then turns into an extra read/error path instead of returning normally.Proposed fix
pub async fn run_server(mut server: keepalive::Server) -> Result<(), Error> { loop { debug!("waiting keepalive request"); server .keepalive_roundtrip() .await .map_err(Error::KeepAliveServerLoop)?; + + if server.is_done() { + return Ok(()); + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pallas-network/src/facades.rs` around lines 97 - 106, The server keep-alive loop in run_server keeps calling keepalive_roundtrip() even after the protocol transitions to Done; update run_server (the function taking server: keepalive::Server) to detect the protocol termination and exit the loop normally instead of invoking another roundtrip. Concretely, call or inspect the keepalive API that reports state (e.g. recv_keepalive_request() or a returned status from keepalive_roundtrip()), match the result/state and break/return Ok(()) when it indicates Done, otherwise continue and map errors to Error::KeepAliveServerLoop as before.
236-244:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
with_chainsyncis unsound and appears to be unused.
tokio::spawnrequiresSend + 'staticfutures. Any closure capturingop(&mut self.chainsync)that awaits will borrow a non-'staticreference and cannot satisfy this bound, making the helper effectively unusable. Additionally, the method is never called anywhere in the codebase. Either remove it as dead code or, if intended as public API, fix it by awaiting inline:Corrected implementation
- pub async fn with_chainsync<T, O, Fut>(&mut self, op: T) -> tokio::task::JoinHandle<O> + pub async fn with_chainsync<T, O, Fut>(&mut self, op: T) -> O where T: FnOnce(&mut chainsync::N2NClient) -> Fut, - Fut: std::future::Future<Output = O> + Send + 'static, - O: Send + 'static, + Fut: std::future::Future<Output = O>, { - tokio::spawn(op(&mut self.chainsync)) + op(&mut self.chainsync).await }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pallas-network/src/facades.rs` around lines 236 - 244, with_chainsync is unsound because it calls tokio::spawn(op(&mut self.chainsync)), which requires the spawned future to be Send+'static but the closure borrows &mut self.chainsync; either delete this unused method or change it so it does not spawn a non-'static borrow: implement it to await inline (call op(&mut self.chainsync).await and return O synchronously/async from with_chainsync) or make chainsync shareable (e.g., store Arc<Mutex<chainsync::N2NClient>> as self.chainsync and clone that into the spawned task) so the future is Send+'static; update references to with_chainsync accordingly.pallas-txbuilder/src/transaction/model.rs (1)
890-923:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't
unwrap()an empty witness set on signature removal.If this removes the last witness,
NonEmptySet::from_vec(vkey_witnesses)returnsNoneand Line 921 panics. Removing the final signature should clearvkeywitness, not crash.Proposed fix
- tx.transaction_witness_set.vkeywitness = - Some(NonEmptySet::from_vec(vkey_witnesses).unwrap()); + tx.transaction_witness_set.vkeywitness = + NonEmptySet::from_vec(vkey_witnesses);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pallas-txbuilder/src/transaction/model.rs` around lines 890 - 923, The Conway branch of remove_signature currently always calls NonEmptySet::from_vec(...).unwrap() when reconstructing transaction_witness_set.vkeywitness, which will panic if the retained vkey_witnesses vector is empty; change the logic in remove_signature (BuilderEra::Conway) to check if vkey_witnesses.is_empty() and set tx.transaction_witness_set.vkeywitness = None in that case, otherwise set it to Some(NonEmptySet::from_vec(vkey_witnesses).unwrap()); ensure you reference the same variables (new_sigs, pk, tx, vkey_witnesses, self.tx_bytes) so the witness list is safely cleared instead of panic-ing when the last signature is removed.pallas-txbuilder/src/lib.rs (1)
55-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the
MalformedScriptdisplay string.This variant documents a script-decoding failure, but Line 59 still renders
"Transaction has no inputs". That will misreport the actual cause to callers.Proposed fix
- #[error("Transaction has no inputs")] + #[error("Could not decode script bytes")]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pallas-txbuilder/src/lib.rs` around lines 55 - 60, The error variant TxBuilderError::MalformedScript currently has the wrong display string; update the #[error(...)] attribute on the MalformedScript enum variant so it describes a script decoding failure (e.g., "Provided bytes could not be decoded into a script" or similar) instead of "Transaction has no inputs" to ensure the reported error matches the variant meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pallas-txbuilder/src/transaction/model.rs`:
- Around line 124-128: The remove_output builder currently calls Vec::remove
which will panic for empty outputs or out-of-range index; change remove_output
to guard by checking self.outputs (use self.outputs.as_mut() or
unwrap_or_default then operate on the mutable vec) and only call remove when
index < txouts.len(), otherwise leave self.outputs unchanged (no-op) and return
self; reference the remove_output method, the outputs field, and Vec::remove in
your change and keep the method signature the same.
In `@pallas-utxorpc/src/lib.rs`:
- Around line 96-104: The change adds a required method get_slot_timestamp to
the public LedgerContext trait which is a breaking change for existing
implementations; make it non-breaking by providing a default implementation on
the LedgerContext trait (e.g., returning None) so existing types implementing
LedgerContext (which currently implement get_utxos) continue to compile, or
alternatively mark it as a new required method only after a major version bump
and document the breaking change—update the trait declaration for LedgerContext
to supply a default for get_slot_timestamp so callers can opt-in to override it.
In `@pallas/src/lib.rs`:
- Around line 239-262: The new nested pub mod hardano::configs accidentally
feature-gates the configs API; restore a non-feature-gated export so consumers
can use pallas_configs without enabling hardano by either (A) adding a top-level
pub mod configs that re-exports pallas_configs (pub use pallas_configs::*;)
outside the #[cfg(feature = "hardano")] block, or (B) split the feature flags
and make configs gated by its own feature (e.g., #[cfg(feature = "configs")] for
the configs module) while keeping hardano separately gated; update the existing
hardano::configs to remain for compatibility if desired but ensure the top-level
pub mod configs or separate feature exposes pallas_configs unconditionally or
under its own feature.
---
Outside diff comments:
In `@pallas-network/src/facades.rs`:
- Around line 97-106: The server keep-alive loop in run_server keeps calling
keepalive_roundtrip() even after the protocol transitions to Done; update
run_server (the function taking server: keepalive::Server) to detect the
protocol termination and exit the loop normally instead of invoking another
roundtrip. Concretely, call or inspect the keepalive API that reports state
(e.g. recv_keepalive_request() or a returned status from keepalive_roundtrip()),
match the result/state and break/return Ok(()) when it indicates Done, otherwise
continue and map errors to Error::KeepAliveServerLoop as before.
- Around line 236-244: with_chainsync is unsound because it calls
tokio::spawn(op(&mut self.chainsync)), which requires the spawned future to be
Send+'static but the closure borrows &mut self.chainsync; either delete this
unused method or change it so it does not spawn a non-'static borrow: implement
it to await inline (call op(&mut self.chainsync).await and return O
synchronously/async from with_chainsync) or make chainsync shareable (e.g.,
store Arc<Mutex<chainsync::N2NClient>> as self.chainsync and clone that into the
spawned task) so the future is Send+'static; update references to with_chainsync
accordingly.
In `@pallas-network/src/miniprotocols/handshake/server.rs`:
- Around line 91-98: In receive_proposed_versions (method on the handshake
server) the unexpected-message branch currently returns Error::InvalidOutbound
but this is a message we received from the peer, so change that to return
Error::InvalidInbound; update the match arm in receive_proposed_versions to
construct and return Error::InvalidInbound instead of InvalidOutbound to
correctly classify the error for downstream debugging and logs.
In `@pallas-network/src/miniprotocols/txmonitor/client.rs`:
- Around line 7-222: Add a public async method named done(&mut self) ->
Result<(), Error> to impl Client that constructs Message::Done, calls
self.send_message(&msg).await? to perform the send (this will enforce
agency/outbound checks), then sets self.0 = State::Done and returns Ok(());
place it alongside the other public API methods (e.g., after release or with
other query methods). Ensure the method signature and state transition match the
existing pattern used by send_acquire/release/query_* and that Message::Done is
the variant validated by assert_outbound_state.
In `@pallas-txbuilder/src/lib.rs`:
- Around line 55-60: The error variant TxBuilderError::MalformedScript currently
has the wrong display string; update the #[error(...)] attribute on the
MalformedScript enum variant so it describes a script decoding failure (e.g.,
"Provided bytes could not be decoded into a script" or similar) instead of
"Transaction has no inputs" to ensure the reported error matches the variant
meaning.
In `@pallas-txbuilder/src/transaction/model.rs`:
- Around line 890-923: The Conway branch of remove_signature currently always
calls NonEmptySet::from_vec(...).unwrap() when reconstructing
transaction_witness_set.vkeywitness, which will panic if the retained
vkey_witnesses vector is empty; change the logic in remove_signature
(BuilderEra::Conway) to check if vkey_witnesses.is_empty() and set
tx.transaction_witness_set.vkeywitness = None in that case, otherwise set it to
Some(NonEmptySet::from_vec(vkey_witnesses).unwrap()); ensure you reference the
same variables (new_sigs, pk, tx, vkey_witnesses, self.tx_bytes) so the witness
list is safely cleared instead of panic-ing when the last signature is removed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2923dfed-7c55-4608-9d5b-ebcc0a0e37c4
📒 Files selected for processing (63)
.gitignoreREADME.mdpallas-addresses/README.mdpallas-addresses/src/lib.rspallas-bech32/src/lib.rspallas-codec/src/lib.rspallas-configs/src/lib.rspallas-crypto/src/hash/mod.rspallas-crypto/src/lib.rspallas-hardano/src/lib.rspallas-math/src/lib.rspallas-network/src/facades.rspallas-network/src/lib.rspallas-network/src/miniprotocols/blockfetch/client.rspallas-network/src/miniprotocols/blockfetch/protocol.rspallas-network/src/miniprotocols/blockfetch/server.rspallas-network/src/miniprotocols/chainsync/buffer.rspallas-network/src/miniprotocols/chainsync/client.rspallas-network/src/miniprotocols/chainsync/protocol.rspallas-network/src/miniprotocols/chainsync/server.rspallas-network/src/miniprotocols/common.rspallas-network/src/miniprotocols/handshake/client.rspallas-network/src/miniprotocols/handshake/mod.rspallas-network/src/miniprotocols/handshake/n2c.rspallas-network/src/miniprotocols/handshake/n2n.rspallas-network/src/miniprotocols/handshake/protocol.rspallas-network/src/miniprotocols/handshake/server.rspallas-network/src/miniprotocols/keepalive/client.rspallas-network/src/miniprotocols/keepalive/protocol.rspallas-network/src/miniprotocols/keepalive/server.rspallas-network/src/miniprotocols/localmsgnotification/client.rspallas-network/src/miniprotocols/localmsgnotification/protocol.rspallas-network/src/miniprotocols/localmsgnotification/server.rspallas-network/src/miniprotocols/localmsgsubmission/protocol.rspallas-network/src/miniprotocols/localstate/client.rspallas-network/src/miniprotocols/localstate/mod.rspallas-network/src/miniprotocols/localstate/protocol.rspallas-network/src/miniprotocols/localstate/queries_v16/primitives.rspallas-network/src/miniprotocols/localstate/server.rspallas-network/src/miniprotocols/localtxsubmission/client.rspallas-network/src/miniprotocols/localtxsubmission/mod.rspallas-network/src/miniprotocols/localtxsubmission/primitives.rspallas-network/src/miniprotocols/localtxsubmission/server.rspallas-network/src/miniprotocols/mod.rspallas-network/src/miniprotocols/peersharing/client.rspallas-network/src/miniprotocols/peersharing/protocol.rspallas-network/src/miniprotocols/peersharing/server.rspallas-network/src/miniprotocols/txmonitor/client.rspallas-network/src/miniprotocols/txmonitor/protocol.rspallas-network/src/miniprotocols/txsubmission/client.rspallas-network/src/miniprotocols/txsubmission/protocol.rspallas-network/src/miniprotocols/txsubmission/server.rspallas-network/src/multiplexer.rspallas-network2/src/lib.rspallas-primitives/src/lib.rspallas-traverse/src/lib.rspallas-txbuilder/src/conway.rspallas-txbuilder/src/lib.rspallas-txbuilder/src/transaction/mod.rspallas-txbuilder/src/transaction/model.rspallas-utxorpc/src/lib.rspallas-validate/src/lib.rspallas/src/lib.rs
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.
Adds comprehensive crate- and module-level documentation across the workspace, fixes rustdoc warnings, and moves
pallas::interop::configsinsidehardano.Summary by CodeRabbit
New Features
Network::Other(u8)support for custom networks in address handling.noncein crypto,math_dashuin math,multiplexerin network, andconwayin primitives.Documentation
Chores