fix: Improve conformance tests coverage - #853
Conversation
📝 WalkthroughWalkthroughAdds asynchronous per-pool off‑chain metadata fetching and propagation into transaction/pool models, introduces a hacks module for domain-aware genesis handling, expands Conway DRep voting collection, adjusts epoch/pparams and cost-model serialization, and moves historical UTXO lookup into DomainAdapter. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Routes as Routes/Txs
participant Builder as TxModelBuilder
participant API as Offchain API
Client->>Routes: Request tx by hash
Routes->>Builder: create builder (with_chain/with_network)
Builder->>Builder: init pool_metadata HashMap
Routes->>Builder: await Builder.fetch_pool_metadata()
Builder->>API: parallel fetch per-pool URLs (join_all)
API-->>Builder: return PoolOffchainMetadata results
Builder->>Builder: populate pool_metadata map
Builder->>Builder: wire offchain into PoolUpdateModelBuilder
Builder-->>Routes: enriched tx model
Routes-->>Client: response with pool metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/minibf/src/mapping.rs`:
- Around line 1652-1659: Remove the debug macro call dbg!(&meta) in the closure
that builds pool updates: inside the builder.map closure created from
PoolUpdateModelBuilder::new(...) (the block that fetches meta from
self.pool_metadata and calls x.with_offchain(meta)), delete the dbg!(&meta);
line so the code no longer prints to stderr during pool registration processing;
if you need non-debug logging, replace it with a proper logger call instead of
dbg!.
- Around line 136-150: The pool_offchain_metadata function currently fetches
user-controlled URLs directly, risking SSRF; before calling
client.get(url).send() in pool_offchain_metadata, parse and validate the URL
(use url::Url) to allow only http/https schemes, reject file:// and other
schemes, and ensure the host isn't a loopback, link-local, or private/reserved
IP or resolves to such IPs (perform DNS resolution and check IP ranges, and
reject raw IP literals in private ranges). If validation fails, return None;
otherwise proceed to build the reqwest client and send the request. Ensure
validation logic is applied inside the pool_offchain_metadata function prior to
making the outbound request.
| pub async fn pool_offchain_metadata(url: &str) -> Option<PoolOffchainMetadata> { | ||
| let client = reqwest::Client::builder() | ||
| .timeout(Duration::from_secs(5)) | ||
| .user_agent("Dolos MiniBF") | ||
| .build() | ||
| .ok()?; | ||
|
|
||
| let res = client.get(url).send().await.ok()?; | ||
|
|
||
| if res.status() != StatusCode::OK { | ||
| return None; | ||
| } | ||
|
|
||
| res.json().await.ok() | ||
| } |
There was a problem hiding this comment.
Consider URL validation to mitigate SSRF risk.
The pool_offchain_metadata function directly fetches from user-controlled URLs stored in pool registration certificates. While pool metadata URLs are part of on-chain data, consider adding URL validation (e.g., disallowing private IP ranges, localhost, file:// schemes) to reduce server-side request forgery (SSRF) exposure.
🛡️ Suggested URL validation approach
pub async fn pool_offchain_metadata(url: &str) -> Option<PoolOffchainMetadata> {
+ // Basic URL validation to prevent SSRF
+ let parsed = url::Url::parse(url).ok()?;
+ if !matches!(parsed.scheme(), "http" | "https") {
+ return None;
+ }
+
let client = reqwest::Client::builder()
.timeout(Duration::from_secs(5))
.user_agent("Dolos MiniBF")
.build()
.ok()?;🤖 Prompt for AI Agents
In `@crates/minibf/src/mapping.rs` around lines 136 - 150, The
pool_offchain_metadata function currently fetches user-controlled URLs directly,
risking SSRF; before calling client.get(url).send() in pool_offchain_metadata,
parse and validate the URL (use url::Url) to allow only http/https schemes,
reject file:// and other schemes, and ensure the host isn't a loopback,
link-local, or private/reserved IP or resolves to such IPs (perform DNS
resolution and check IP ranges, and reject raw IP literals in private ranges).
If validation fails, return None; otherwise proceed to build the reqwest client
and send the request. Ensure validation logic is applied inside the
pool_offchain_metadata function prior to making the outbound request.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/minibf/src/routes/blocks.rs`:
- Around line 286-290: The match on domain.genesis().shelley.network_magic uses
todo!() which will panic for None (mainnet) or unexpected values; change this to
return a proper error instead of panicking. Replace the _ => todo!() arm in the
block_0 initialization with an Err variant (or create a new Error enum variant)
that wraps a descriptive message about unsupported/unknown network_magic,
ensuring it composes with the existing .map_err(Error::Code)? pattern used for
block_0_preprod and block_0_preview; keep references to block_0,
domain.genesis().shelley.network_magic, block_0_preprod, block_0_preview, and
Error::Code so callers can handle the failure rather than crashing.
- Around line 217-220: The handler currently returns the synthetic block 0 only
for preprod (network_magic == Some(1)) but not for preview (Some(2)), causing
load_block_by_hash_or_number to return NOT_FOUND; add an equivalent early return
when Either::Right(0) == hash_or_number &&
domain.genesis().shelley.network_magic == Some(2) that returns
Ok(Json(block_0_preview(&domain)?)) (mirroring the block_0_preprod branch), and
ensure the surrounding if/return block uses the same brace placement and
indentation style as the existing preprod branch and the pagination code to keep
formatting consistent.
🧹 Nitpick comments (5)
crates/minibf/src/mapping.rs (3)
128-134: Consider making fields optional to handle incomplete metadata JSON.All fields in
PoolOffchainMetadataare requiredStringtypes. If a pool operator's metadata JSON is missing any of these fields (which is common), theres.json().awaitdeserialization will fail silently (returningNone). Consider usingOption<String>with serde defaults to be more resilient:♻️ Suggested change for robustness
#[derive(Serialize, Deserialize, Clone, Debug)] pub struct PoolOffchainMetadata { + #[serde(default)] - pub name: String, + pub name: Option<String>, + #[serde(default)] - pub description: String, + pub description: Option<String>, + #[serde(default)] - pub ticker: String, + pub ticker: Option<String>, + #[serde(default)] - pub homepage: String, + pub homepage: Option<String>, }
577-628: Consider limiting concurrent metadata fetches.The
fetch_pool_metadatafunction usesjoin_allto fetch metadata for all pool registrations in parallel without any concurrency limit. While typically there are few pool registrations per transaction, a transaction with many registrations could spawn unbounded concurrent HTTP requests, potentially overwhelming the network or triggering rate limits on metadata servers.♻️ Suggested approach using futures stream with buffer
+use futures::stream::{self, StreamExt}; + pub async fn fetch_pool_metadata(&mut self) -> Result<(), StatusCode> { // ... pool_registrations extraction ... - self.pool_metadata = join_all(pool_registrations.iter().map( - |(pool_hash, url)| async move { - pool_offchain_metadata(url) - .await - .map(|meta| (*pool_hash, meta)) - }, - )) - .await - .into_iter() - .flatten() - .collect(); + self.pool_metadata = stream::iter(pool_registrations.iter()) + .map(|(pool_hash, url)| async move { + pool_offchain_metadata(url) + .await + .map(|meta| (*pool_hash, meta)) + }) + .buffer_unordered(4) // Limit concurrent requests + .filter_map(|x| async { x }) + .collect() + .await; Ok(()) }
588-588: Redundant clone pattern.The pattern
*(**cow).clone()clones the entire certificate struct. Since you only need theoperatorandpool_metadatafields, consider extracting just those references without cloning the whole struct:♻️ Suggested optimization
MultiEraCert::AlonzoCompatible(cow) => { - if let AlonzoCert::PoolRegistration { - operator, - ref pool_metadata, - .. - } = *(**cow).clone() + if let AlonzoCert::PoolRegistration { + operator, + pool_metadata, + .. + } = &***cow { pool_metadata .as_ref() - .map(|meta| (operator, meta.url.clone())) + .map(|meta| (*operator, meta.url.clone())) } else { None } }crates/minibf/src/routes/blocks.rs (2)
68-109: Consider extracting common logic to reduce duplication.
block_0_preprodis nearly identical toblock_0_preview(lines 25-66). The only differences are the hardcodedtime,hash, andnext_blockvalues. The confirmations calculation and utxo summation logic is duplicated.Consider extracting a helper that accepts network-specific parameters:
♻️ Proposed refactor
struct GenesisBlockParams { time: i32, hash: &'static str, next_block: &'static str, } fn block_0_generic<D: Domain>( domain: &Facade<D>, params: GenesisBlockParams, ) -> Result<BlockContent, StatusCode> { let confirmations = MultiEraBlock::decode( &domain .archive() .get_tip() .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)? .ok_or(StatusCode::INTERNAL_SERVER_ERROR)? .1, ) .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)? .header() .number() as i32; let byron_utxos = byron::genesis_utxos(&domain.genesis().byron); let shelley_utxos = shelley::shelley_utxos(&domain.genesis().shelley); Ok(BlockContent { time: params.time, hash: params.hash.to_string(), next_block: Some(params.next_block.to_string()), // ... rest of fields }) }
282-284: Update stale comment.The comment says "Insert block 0 only in preview" but the code now handles both preview (magic 2) and preprod (magic 1).
📝 Suggested comment update
- // Insert block 0 only in preview + // Insert synthetic block 0 for testnets (preprod and preview) if output.len() < pagination.count
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/minibf/src/routes/governance.rs`:
- Around line 43-49: The drep_script match arm currently encodes bech32 using
the original 28-byte payload but returns a 29-byte payload with
DREP_SCRIPT_PREFIX, causing a mismatch; update the bech32 call in the
("drep_script", 28) branch to use the prefixed new_payload (the same bytes you
return) instead of &payload so bech32(..., &new_payload) and the returned
[vec![DREP_SCRIPT_PREFIX], payload].concat() are consistent with the other drep
cases (refer to DREP_SCRIPT_PREFIX, new_payload, and the bech32 call in that
match arm).
| ("drep_script", 28) => Ok(( | ||
| bech32(bech32::Hrp::parse("drep").unwrap(), &payload) | ||
| .map_err(|_| StatusCode::BAD_REQUEST)?, | ||
| [vec![pallas_extras::DREP_SCRIPT_PREFIX], payload].concat(), | ||
| true, | ||
| false, | ||
| )), |
There was a problem hiding this comment.
Inconsistent bech32 encoding for drep_script case.
The drep_script branch encodes the bech32 string with the original 28-byte payload, but returns a 29-byte payload that includes the DREP_SCRIPT_PREFIX. This is inconsistent with the drep/drep_vkh case (lines 33-41) where both the bech32 encoding and returned payload use the prefixed new_payload.
This mismatch means the drep_id string won't correspond to the drep_id_encoded bytes for script DReps, potentially causing incorrect hex representations or lookup issues downstream.
🐛 Proposed fix to use consistent payload for encoding
("drep_script", 28) => Ok((
- bech32(bech32::Hrp::parse("drep").unwrap(), &payload)
- .map_err(|_| StatusCode::BAD_REQUEST)?,
- [vec![pallas_extras::DREP_SCRIPT_PREFIX], payload].concat(),
+ {
+ let new_payload = [vec![pallas_extras::DREP_SCRIPT_PREFIX], payload].concat();
+ (
+ bech32(bech32::Hrp::parse("drep").unwrap(), &new_payload)
+ .map_err(|_| StatusCode::BAD_REQUEST)?,
+ new_payload,
+ )
+ }.0,
+ {
+ let new_payload = [vec![pallas_extras::DREP_SCRIPT_PREFIX], payload].concat();
+ new_payload
+ },
true,
false,
)),Or more cleanly:
-("drep_script", 28) => Ok((
- bech32(bech32::Hrp::parse("drep").unwrap(), &payload)
- .map_err(|_| StatusCode::BAD_REQUEST)?,
- [vec![pallas_extras::DREP_SCRIPT_PREFIX], payload].concat(),
- true,
- false,
-)),
+("drep_script", 28) => {
+ let new_payload = [vec![pallas_extras::DREP_SCRIPT_PREFIX], payload].concat();
+ Ok((
+ bech32(bech32::Hrp::parse("drep").unwrap(), &new_payload)
+ .map_err(|_| StatusCode::BAD_REQUEST)?,
+ new_payload,
+ true,
+ false,
+ ))
+}🤖 Prompt for AI Agents
In `@crates/minibf/src/routes/governance.rs` around lines 43 - 49, The drep_script
match arm currently encodes bech32 using the original 28-byte payload but
returns a 29-byte payload with DREP_SCRIPT_PREFIX, causing a mismatch; update
the bech32 call in the ("drep_script", 28) branch to use the prefixed
new_payload (the same bytes you return) instead of &payload so bech32(...,
&new_payload) and the returned [vec![DREP_SCRIPT_PREFIX], payload].concat() are
consistent with the other drep cases (refer to DREP_SCRIPT_PREFIX, new_payload,
and the bech32 call in that match arm).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/external/smoke.rs`:
- Around line 234-235: Remove the leftover dbg! invocation (dbg!(&after)) from
the test; either delete the dbg! call entirely or replace it with a deliberate
diagnostic such as println! or eprintln! with a clear message referencing the
variable after if you need runtime output, then re-run cargo clippy to ensure
the dbg_macro lint is resolved.
🧹 Nitpick comments (1)
src/adapters/mod.rs (1)
61-85: Track remaining refs to avoid unnecessary WAL scans.Line 82 breaks on
result.len() == refs.len(), so duplicate refs will prevent early exit and can force a full WAL scan. Consider tracking remaining refs and breaking when the set is empty.♻️ Suggested refactor
- let refs_set: std::collections::HashSet<_> = - refs.iter().copied().map(TxoRef::from).collect(); + let mut remaining: std::collections::HashSet<_> = + refs.iter().copied().map(TxoRef::from).collect(); let iter = self.wal().iter_logs(None, None).ok()?; for (_, log) in iter.rev() { for (txo_ref, era_cbor) in &log.inputs { - if refs_set.contains(txo_ref) { + if remaining.remove(txo_ref) { let era = era_cbor.0.try_into().expect("era out of range"); result.insert(txo_ref.clone().into(), (era, era_cbor.1.clone())); } } - if result.len() == refs.len() { + if remaining.is_empty() { break; } }
| dbg!(&after); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove leftover dbg! statement.
The dbg! macro is a debugging artifact that should not be committed. It will be flagged by cargo clippy (via dbg_macro lint). If logging is needed for test diagnostics, consider using println! or eprintln! with a clear message, or remove it entirely.
🧹 Proposed fix
- dbg!(&after);
-
assert!(after.wal.tip_slot.unwrap() >= before.wal.tip_slot.unwrap_or_default() + 20);As per coding guidelines: "Run cargo clippy --workspace --all-targets --all-features and resolve all clippy warnings before committing changes."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dbg!(&after); |
🤖 Prompt for AI Agents
In `@tests/external/smoke.rs` around lines 234 - 235, Remove the leftover dbg!
invocation (dbg!(&after)) from the test; either delete the dbg! call entirely or
replace it with a deliberate diagnostic such as println! or eprintln! with a
clear message referencing the variable after if you need runtime output, then
re-run cargo clippy to ensure the dbg_macro lint is resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minibf/src/routes/blocks.rs (1)
179-181:⚠️ Potential issue | 🟠 MajorHandle numeric genesis (
/blocks/0) consistently in previous/next endpoints.
/blocks/0is synthesized, but/blocks/0/previousand/blocks/0/nextstill go throughload_block_by_hash_or_number, which can return NOT_FOUND when genesis isn’t archived. TreatEither::Right(0)as genesis: previous should return empty; next should use the genesis range path.Suggested fix
let hash_or_number = parse_hash_or_number(&hash_or_number)?; + if matches!(hash_or_number, Either::Right(0)) { + return Ok(Json(Vec::new())); + } let curr = load_block_by_hash_or_number(&domain, &hash_or_number).await?;- let is_genesis = match &hash_or_number { - Either::Left(hash) => { - hacks::is_genesis_hash_for_domain(&domain, hash).map_err(Error::Code)? - } - _ => false, - }; + let is_genesis = match &hash_or_number { + Either::Right(0) => true, + Either::Left(hash) => { + hacks::is_genesis_hash_for_domain(&domain, hash).map_err(Error::Code)? + } + _ => false, + };Also applies to: 258-263
🤖 Fix all issues with AI agents
In `@crates/minibf/src/routes/blocks.rs`:
- Around line 228-236: The code determines whether to include genesis in the
"previous" pagination using genesis_index = curr_number but that is off-by-one;
change genesis_index to curr_number.saturating_sub(1) (or otherwise subtract 1
safely) so the zero-based previous range check (from, to =
from.saturating_add(pagination.count)) correctly sets genesis_in_range and
allows the block_0.take() -> output.push(genesis) path to run for ranges that
should include genesis; update the genesis_index computation where it is
declared so genesis_in_range logic and use of block_0/output remain correct.
🧹 Nitpick comments (2)
crates/minibf/src/mapping.rs (2)
583-612: Consider avoiding unnecessary clone when pattern matching certificates.The pattern
*(**cow).clone()clones the entire certificate just to pattern match on its variant. This could be optimized by matching on references instead.♻️ Suggested optimization
MultiEraCert::AlonzoCompatible(cow) => { - if let AlonzoCert::PoolRegistration { - operator, - ref pool_metadata, - .. - } = *(**cow).clone() - { - pool_metadata - .as_ref() - .map(|meta| (operator, meta.url.clone())) + if let AlonzoCert::PoolRegistration { + operator, + pool_metadata, + .. + } = &***cow + { + pool_metadata + .as_ref() + .map(|meta| (*operator, meta.url.clone())) } else { None } } MultiEraCert::Conway(cow) => { - if let ConwayCert::PoolRegistration { - operator, - ref pool_metadata, - .. - } = *(**cow).clone() - { - pool_metadata - .as_ref() - .map(|meta| (operator, meta.url.clone())) + if let ConwayCert::PoolRegistration { + operator, + pool_metadata, + .. + } = &***cow + { + pool_metadata + .as_ref() + .map(|meta| (*operator, meta.url.clone())) } else { None } }
1986-2025: Consider extracting repeated pattern into a helper.The same pattern for handling zero values in genesis/epoch boundary blocks is repeated three times (epoch_slot, slot, height). While the current code is correct and readable, a helper function could reduce duplication.
♻️ Optional helper extraction
fn genesis_aware_value(&self, value: i32) -> Option<i32> { match value { 0 if matches!( self.block, MultiEraBlock::EpochBoundary(_) | MultiEraBlock::Byron(_) ) => None, x => Some(x), } }Then use:
epoch_slot: epoch_slot.map(|x| x as i32).and_then(|x| self.genesis_aware_value(x))
| let to = from.saturating_add(pagination.count); | ||
| let genesis_index = curr_number; | ||
| let genesis_in_range = from <= genesis_index && genesis_index < to; | ||
|
|
||
| if genesis_in_range { | ||
| if let Some(genesis) = block_0.take() { | ||
| output.push(genesis); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix off‑by‑one when deciding whether to include genesis in previous pagination.
genesis_index should be curr_number - 1 (zero‑based offsets for “previous” list). Current logic skips genesis for ranges like from=5, count=5 when it should be included.
Suggested fix
- let genesis_index = curr_number;
- let genesis_in_range = from <= genesis_index && genesis_index < to;
+ let genesis_index = curr_number.saturating_sub(1);
+ let genesis_in_range =
+ curr_number > 0 && from <= genesis_index && genesis_index < to;🤖 Prompt for AI Agents
In `@crates/minibf/src/routes/blocks.rs` around lines 228 - 236, The code
determines whether to include genesis in the "previous" pagination using
genesis_index = curr_number but that is off-by-one; change genesis_index to
curr_number.saturating_sub(1) (or otherwise subtract 1 safely) so the zero-based
previous range check (from, to = from.saturating_add(pagination.count))
correctly sets genesis_in_range and allows the block_0.take() ->
output.push(genesis) path to run for ranges that should include genesis; update
the genesis_index computation where it is declared so genesis_in_range logic and
use of block_0/output remain correct.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/minibf/src/mapping.rs`:
- Around line 653-656: Remove the temporary debug print so stake registration
processing doesn't spam stderr: delete the dbg!(®) call inside the loop that
handles certs (the branch using pallas_extras::cert_as_stake_registration and
creating key via minicbor::to_vec(®)). If you still need visibility, replace
it with a proper logger call (e.g., tracing::debug!) but do not leave dbg!(®)
in the final code.
- Around line 653-699: Remove the dbg!(®) call and for stake and DRep
registration handling replicate the pool-registration on-chain check: before
adding key_deposit or reg.deposit consult
facade.read_cardano_entity::<AccountState> (for stake registrations using
reg/cred key) and facade.read_cardano_entity::<DRepState> (for DRep
registrations using reg.cred) and, like the PoolState branch in the loop, only
add the deposit when the on-chain state indicates the registration isn't already
current (perform the same self.block.slot() vs existing.register_slot comparison
and match Some/None as done for PoolState); keep using the repeated set for
intra-transaction deduplication.
| for cert in self.tx()?.certs() { | ||
| if let Some(reg) = pallas_extras::cert_as_stake_registration(&cert) { | ||
| dbg!(®); | ||
| let key = minicbor::to_vec(®).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; |
There was a problem hiding this comment.
Remove debug macro before merge.
dbg!(®) will spam stderr on every stake registration cert processed.
🐛 Suggested fix
- dbg!(®);🤖 Prompt for AI Agents
In `@crates/minibf/src/mapping.rs` around lines 653 - 656, Remove the temporary
debug print so stake registration processing doesn't spam stderr: delete the
dbg!(®) call inside the loop that handles certs (the branch using
pallas_extras::cert_as_stake_registration and creating key via
minicbor::to_vec(®)). If you still need visibility, replace it with a proper
logger call (e.g., tracing::debug!) but do not leave dbg!(®) in the final
code.
| for cert in self.tx()?.certs() { | ||
| if let Some(reg) = pallas_extras::cert_as_stake_registration(&cert) { | ||
| dbg!(®); | ||
| let key = minicbor::to_vec(®).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| if repeated.insert((0, key.clone())) { | ||
| out += key_deposit; | ||
| } | ||
| } | ||
|
|
||
| if let Some(reg) = pallas_extras::cert_as_pool_registration(&cert) { | ||
| if repeated.insert((1, reg.operator.to_vec())) { | ||
| match facade | ||
| .read_cardano_entity::<PoolState>(reg.operator.as_slice()) | ||
| .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)? | ||
| { | ||
| Some(pool) => { | ||
| if self.block.slot() <= pool.register_slot { | ||
| out += pool_deposit | ||
| } | ||
| } | ||
| None => out += pool_deposit, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if let Some(reg) = pallas_extras::cert_as_drep_registration(&cert) { | ||
| let key = | ||
| minicbor::to_vec(®.cred).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| if repeated.insert((2, key)) { | ||
| out += reg.deposit as i64; | ||
| } | ||
| } | ||
|
|
||
| if let Some(reg) = pallas_extras::cert_as_stake_deregistration(&cert) { | ||
| let key = minicbor::to_vec(®).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| if repeated.insert((3, key.clone())) { | ||
| out -= key_deposit; | ||
| } | ||
| } | ||
|
|
||
| if let Some(reg) = pallas_extras::cert_as_drep_unregistration(&cert) { | ||
| let key = | ||
| minicbor::to_vec(®.cred).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| if repeated.insert((4, key)) { | ||
| out -= reg.deposit as i64; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and locate related state types
find crates/minibf/src -type f -name "*.rs" | head -20Repository: txpipe/dolos
Length of output: 754
🏁 Script executed:
# Search for AccountState and DRepState in the codebase
rg "AccountState|DRepState" --type rust -iRepository: txpipe/dolos
Length of output: 17618
🏁 Script executed:
# Examine the mapping.rs file more closely around the suspicious code
wc -l crates/minibf/src/mapping.rsRepository: txpipe/dolos
Length of output: 89
🏁 Script executed:
# Look for similar patterns with PoolState to understand the approach
rg "read_cardano_entity.*PoolState" --type rust -A 5 -B 5Repository: txpipe/dolos
Length of output: 911
🏁 Script executed:
# Check the dbg! macro usage and code maturity
rg "dbg!" crates/minibf/src/mapping.rs -nRepository: txpipe/dolos
Length of output: 87
🏁 Script executed:
# Search for re-registration related comments or patterns
rg "re.*registr|duplicate.*registr|repeat.*registr" --type rust -iRepository: txpipe/dolos
Length of output: 6785
🏁 Script executed:
# Read lines 640-710 to get full context
sed -n '640,710p' crates/minibf/src/mapping.rsRepository: txpipe/dolos
Length of output: 2736
🏁 Script executed:
# Find the function signature and understand the facade type
sed -n '600,660p' crates/minibf/src/mapping.rsRepository: txpipe/dolos
Length of output: 2103
🏁 Script executed:
# Check the governance route to see how DRepState is queried
sed -n '1,100p' crates/minibf/src/routes/governance.rsRepository: txpipe/dolos
Length of output: 3209
🏁 Script executed:
# Search for how the facade is used in the txs route for comparison
grep -A 10 "read_cardano_entity" crates/minibf/src/routes/txs.rs | head -40Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Look at the governance route to see DRepState read pattern
sed -n '80,150p' crates/minibf/src/routes/governance.rsRepository: txpipe/dolos
Length of output: 2075
🏁 Script executed:
# Search for re-registration or double registration patterns in Cardano code
rg "re.*register|double.*register|already.*register" crates/cardano --type rust -i -B 2 -A 2Repository: txpipe/dolos
Length of output: 8327
🏁 Script executed:
# Check if there are any comments about re-registration in roll/accounts.rs
sed -n '1,100p' crates/cardano/src/roll/accounts.rsRepository: txpipe/dolos
Length of output: 2983
🏁 Script executed:
# Look at StakeRegistration and StakeDeregistration implementations
rg "impl.*StakeRegistration|impl.*StakeDeregistration" crates/cardano/src/roll/accounts.rs -A 20Repository: txpipe/dolos
Length of output: 2302
🏁 Script executed:
# Check if there are any comments about re-registration behavior or Cardano rules
rg "re.*register|cannot.*register.*twice" crates/cardano --type rust -i -B 3 -A 3Repository: txpipe/dolos
Length of output: 10727
🏁 Script executed:
# Look at the Cardano ledger validation to understand if re-registrations are caught
rg "already.*registered|duplicate.*registration" crates/cardano --type rust -i -B 2 -A 2Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Check how compute_deposit is used - is it validating or computing estimates?
rg "compute_deposit" crates/minibf/src --type rust -B 5 -A 2Repository: txpipe/dolos
Length of output: 939
🏁 Script executed:
# Look at the full function to see if there's a reason to only check pools
sed -n '590,610p' crates/minibf/src/mapping.rsRepository: txpipe/dolos
Length of output: 875
🏁 Script executed:
# Check what PoolState.register_slot tracks to understand the pool check logic
rg "register_slot" crates/cardano/src --type rust -B 2 -A 2Repository: txpipe/dolos
Length of output: 996
🏁 Script executed:
# Search for validation of certificate duplicates
rg "cert.*duplicate|certificate.*duplicate" crates/cardano --type rust -i -B 2 -A 2Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Check if there are ledger validation rules in the codebase
rg "validate.*cert|cert.*valid" crates/cardano/src/validate.rs --type rust -B 2 -A 2Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Look for what the repeated set is actually guarding against
sed -n '650,700p' crates/minibf/src/mapping.rs | cat -nRepository: txpipe/dolos
Length of output: 2527
🏁 Script executed:
# Search for Cardano ledger specification comments about re-registrations
rg "re.*register|duplicate.*register|multiple.*register" crates --type rust -iRepository: txpipe/dolos
Length of output: 2904
🏁 Script executed:
# Check if AccountState has a registered_at field and what it tracks
rg "registered_at|deregistered_at" crates/cardano/src/model.rs -B 1 -A 1Repository: txpipe/dolos
Length of output: 697
🏁 Script executed:
# Check the pool registration check more carefully - what does register_slot mean
sed -n '620,680p' crates/minibf/src/mapping.rsRepository: txpipe/dolos
Length of output: 2283
🏁 Script executed:
# Look at how register_slot is set when a pool is updated/re-registered
rg "PoolRegistration" crates/cardano/src/roll/pools.rs -B 3 -A 15Repository: txpipe/dolos
Length of output: 2475
🏁 Script executed:
# Check if a stake account can be re-registered according to Cardano rules
rg "StakeRegistration" crates/cardano/src/roll/accounts.rs -B 5 -A 25 | head -80Repository: txpipe/dolos
Length of output: 2689
🏁 Script executed:
# Verify whether is_registered() method checks both registered_at and deregistered_at
sed -n '1,50p' crates/cardano/src/model.rs | grep -A 10 "is_registered"Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Search for any ledger validation that prevents double registration
rg "cannot.*register|already.*registered" --type rust -iRepository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Check the Cardano spec or comments about re-registration rules
rg "re.*register.*allowed|re.*register.*denied|re.*register.*rule" crates --type rust -iRepository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Verify the exact logic of pool register_slot check - when would it NOT charge deposit
sed -n '668,680p' crates/minibf/src/mapping.rsRepository: txpipe/dolos
Length of output: 588
🏁 Script executed:
# Look for how AccountState is used in the facade/domain to understand if it's queried
rg "read_cardano_entity.*AccountState" crates/minibf/src --type rust -B 2 -A 5Repository: txpipe/dolos
Length of output: 598
🏁 Script executed:
# Check if there are any tests for compute_deposit that show expected behavior
fd "test" crates/minibf/src --type f | xargs grep -l "compute_deposit" 2>/dev/nullRepository: txpipe/dolos
Length of output: 40
🏁 Script executed:
# Look for any Cardano ledger documentation references in code
find crates -name "*.rs" -type f | xargs grep -l "ledger.*rule\|ledger.*spec" 2>/dev/null | head -5Repository: txpipe/dolos
Length of output: 114
🏁 Script executed:
# Check if there's a spec URL or reference
rg "cardano.*spec|ledger.*rule|protocol.*rule" crates --type rust -i | head -20Repository: txpipe/dolos
Length of output: 1525
🏁 Script executed:
# Look at the actual test case usage of compute_deposit in the repo
find . -name "*.rs" -type f -exec grep -l "compute_deposit" {} \; | head -10Repository: txpipe/dolos
Length of output: 120
🏁 Script executed:
# Check the pallas library usage to understand validation
rg "pallas.*validate|validate.*cert" crates --type rust -i -B 1 -A 1 | head -30Repository: txpipe/dolos
Length of output: 1571
🏁 Script executed:
# Search for any discussion about re-registration in comments
rg "re.*regist" crates --type rust -C 3 -i | grep -E "(TODO|FIXME|NOTE|re.*regist)" | head -20Repository: txpipe/dolos
Length of output: 2236
🏁 Script executed:
# Check if there's a comment explaining the pool register_slot logic
sed -n '660,690p' crates/minibf/src/mapping.rs | grep -B 5 -A 10 "register_slot"Repository: txpipe/dolos
Length of output: 702
🏁 Script executed:
# Look for any Cardano specification documentation or references in README or docs
find . -name "README*" -o -name "*.md" | xargs grep -i "re.*register\|deposit" 2>/dev/null | head -10Repository: txpipe/dolos
Length of output: 780
🏁 Script executed:
# Check if the Cardano crate has any documentation about certificate validation
head -100 crates/cardano/src/lib.rsRepository: txpipe/dolos
Length of output: 3117
🏁 Script executed:
# Check if there are integration tests that show expected behavior
find crates -name "*.rs" -type f -path "*/tests/*" -exec grep -l "compute_deposit\|registration" {} \;Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Verify what the pool check is really guarding against
sed -n '665,680p' crates/minibf/src/mapping.rsRepository: txpipe/dolos
Length of output: 766
🏁 Script executed:
# Look at the CHANGELOG to understand the history of this code
grep -B 3 -A 3 "deposit" CHANGELOG.md | head -40Repository: txpipe/dolos
Length of output: 985
🏁 Script executed:
# Check if there's any explanation in the PR or commit that added this
git log --oneline --all --grep="deposit" 2>/dev/null | head -20Repository: txpipe/dolos
Length of output: 290
🏁 Script executed:
# Verify the logic once more - what does the slot comparison mean?
# If current slot is BEFORE or EQUAL to register_slot, charge deposit
# If current slot is AFTER register_slot, don't charge (meaning pool was already registered)
echo "Pool logic analysis:"
echo " If block.slot <= pool.register_slot: charge deposit"
echo " If block.slot > pool.register_slot: DON'T charge (re-registration)"
echo ""
echo "Stake/DRep logic: only checks repeated set (same-tx duplicates), no on-chain state check"Repository: txpipe/dolos
Length of output: 289
🏁 Script executed:
# Double-check the pool registration logic with more context
sed -n '665,680p' crates/minibf/src/mapping.rs | cat -nRepository: txpipe/dolos
Length of output: 878
🏁 Script executed:
# Verify from the git commit what the actual fix was
git show e296c23 --stat 2>/dev/null || echo "Cannot access git"Repository: txpipe/dolos
Length of output: 26657
🏁 Script executed:
# Check if there are any comments explaining the slot comparison logic
rg "register_slot" crates/minibf/src/mapping.rs -B 3 -A 3Repository: txpipe/dolos
Length of output: 371
Remove dbg!() macro and add on-chain state checks for stake/DRep registrations.
Pool registrations check PoolState.register_slot to avoid recharging deposits on re-registration, but stake/DRep registrations don't check AccountState or DRepState. The repeated set only prevents duplicates within the current transaction. To handle re-registrations across transactions, query the on-chain state before adding deposits, similar to the pool registration pattern.
🤖 Prompt for AI Agents
In `@crates/minibf/src/mapping.rs` around lines 653 - 699, Remove the dbg!(®)
call and for stake and DRep registration handling replicate the
pool-registration on-chain check: before adding key_deposit or reg.deposit
consult facade.read_cardano_entity::<AccountState> (for stake registrations
using reg/cred key) and facade.read_cardano_entity::<DRepState> (for DRep
registrations using reg.cred) and, like the PoolState branch in the loop, only
add the deposit when the on-chain state indicates the registration isn't already
current (perform the same self.block.slot() vs existing.register_slot comparison
and match Some/None as done for PoolState); keep using the repeated set for
intra-transaction deduplication.
2 participants
/txs/hash/pool_updatesSummary by CodeRabbit
New Features
Bug Fixes
Chores