fix(minibf): use inaccurate but compatible pool fees value - #762
Conversation
WalkthroughThe pull request extends Changes
Sequence DiagramsequenceDiagram
participant Pool as Pool Parameters
participant LogWork as log_work()
participant Log as StakeLog
participant Route as Pool History Route
participant Fees as Fee Calculation
Pool->>LogWork: Provide fixed_cost, margin
LogWork->>Log: Store fixed_cost + margin_cost
Log->>Route: Pass StakeLog record
Route->>Fees: bf_compatible_fees(log)
rect rgb(200, 220, 255)
Note over Fees: margin × rewards<br/>+ fixed_cost<br/>(capped at total_rewards)
end
Fees->>Route: Return computed fee
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve new field additions (routine), CBOR index verification, new computation logic, and modified fee calculation. The modifications are cohesive and logically connected across three files but require careful review of the new fee calculation logic and index ordering. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/minibf/src/routes/pools.rs (1)
210-220: Consider documenting the default margin value assumption.When
margin_costisNone, the code defaults toRational64::from_integer(0), meaning 0% margin. This assumes pools without margin data have no margin. Verify this is the intended behavior and consider adding a comment explaining this assumption.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/cardano/src/model.rs(1 hunks)crates/cardano/src/rupd/mod.rs(2 hunks)crates/minibf/src/routes/pools.rs(2 hunks)
⏰ 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). (4)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Test (macos-14)
- GitHub Check: Test (macos-13)
🔇 Additional comments (3)
crates/cardano/src/rupd/mod.rs (2)
148-149: LGTM!The extraction of
fixed_costandmargin_costfrom pool parameters is straightforward and handles theNonecase appropriately with sensible defaults.
165-166: LGTM!The new fields are correctly passed to the
StakeLogconstructor.crates/minibf/src/routes/pools.rs (1)
274-274: LGTM!Correctly replaced
operator_sharewith the newbf_compatible_feesfunction to compute fees using the blockfrost-compatible formula.
|
|
||
| /// Fixed cost | ||
| #[n(10)] | ||
| pub fixed_cost: u64, | ||
|
|
||
| /// Margin cost | ||
| #[n(11)] | ||
| pub margin_cost: Option<RationalNumber>, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify CBOR backward compatibility for new StakeLog fields.
Adding fixed_cost as a required field (non-Option) at index n(10) may break deserialization of existing StakeLog records that were serialized before this change. When the decoder encounters old records without this field, the behavior depends on minicbor's handling of missing fields.
While the struct derives Default, verify that:
- Existing serialized
StakeLogrecords can still be deserialized - Missing fields are handled gracefully (e.g., using default values)
- Consider making
fixed_costoptional (Option<u64>) if backward compatibility is required
Run the following script to check for existing usage patterns and serialization tests:
🏁 Script executed:
#!/bin/bash
# Search for StakeLog serialization/deserialization tests
rg -nP --type=rust -C5 'StakeLog.*decode|StakeLog.*encode|decode.*StakeLog|encode.*StakeLog'
# Find existing StakeLog records that might need migration
rg -nP --type=rust -C3 'write_log_typed.*StakeLog|read_log.*StakeLog'Length of output: 1646
I'll need to gather more information to verify this backward compatibility concern. Let me search for the complete StakeLog definition and test patterns.
Let me also search the web for information about minicbor's backward compatibility behavior:
🏁 Script executed:
#!/bin/bash
# Find the complete StakeLog struct definition
rg -nA30 'struct StakeLog' --type=rust
# Search for tests related to StakeLog or serialization in test files
fd '.*test\.rs|.*_test\.rs|tests/' --type f | head -20
# Look for test modules within model.rs
rg -nP --type=rust '#\[test\]|#\[cfg\(test\)]' crates/cardano/src/model.rs -A10Length of output: 1600
🏁 Script executed:
#!/bin/bash
# Check minicbor's handling of missing fields during deserialization
# This searches online documentation or crate information
echo "Searching for minicbor usage patterns and missing field behavior..."Length of output: 67
🌐 Web query:
minicbor Rust CBOR missing fields deserialization default values
💡 Result:
Short answer: use minicbor-derive field attributes or serde defaults.
-
With minicbor's derive (minicbor-derive): mark the field with #[cbor(default)] (or #[cbor(skip)] for skipped/always-defaulted fields). The field's type must implement Default; when the field is absent the decode fills it with Default::default(). Optional/Option fields become None if not present. [1][2]
-
If you use the serde bridge (minicbor_serde), use the usual serde mechanisms: #[serde(default)] on fields or implement Default for the type (or use Option which deserializes to None when missing). [5]
References:
[1] minicbor-derive docs (attributes: #[cbor(default)], #[cbor(skip)], Option behavior).
[2] minicbor crate docs / design notes (optional fields default to None, missing fields can use Default).
[5] minicbor_serde docs (use serde::Deserialize/Serialize and serde defaults).
I need to see the complete StakeLog definition including its derive macros to verify the backward compatibility concern.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the StakeLog struct with full definition including derive macros
rg -nB5 -A50 'pub struct StakeLog' crates/cardano/src/model.rs | head -70
# Search for #[cbor(default)] usage in model.rs to see if pattern is used elsewhere
rg -n '#\[cbor\(' crates/cardano/src/model.rs
Length of output: 1329
Add #[cbor(default)] attribute to fixed_cost field to enable backward compatibility.
The backward compatibility concern is valid. StakeLog derives Encode and Decode but does not use #[cbor(default)] on any fields. When deserializing old records serialized before fixed_cost (at index 10) was added, minicbor will fail to deserialize because:
- Old records lack the field at index 10
- Without
#[cbor(default)], the requiredu64field cannot be filled with a default value margin_costasOption<T>is safe (becomesNonewhen missing), butfixed_costas a requiredu64is not
Fix: Add #[cbor(default)] to the fixed_cost field:
/// Fixed cost
#[n(10)]
#[cbor(default)]
pub fixed_cost: u64,This allows minicbor to use Default::default() for old records missing this field, preserving backward compatibility.
🤖 Prompt for AI Agents
In crates/cardano/src/model.rs around lines 335 to 342, the new required u64
field `fixed_cost` (index 10) lacks a #[cbor(default)] attribute causing
minicbor to fail deserializing older records that don't have this field; add the
#[cbor(default)] attribute to the `fixed_cost` field so minicbor will use
Default::default() when the field is absent, preserving backward compatibility
while leaving `margin_cost: Option<RationalNumber>` unchanged.
| // HACK: blockfrost dbsync version computes fees at the SQL query level using | ||
| // the formula: `FLOOR(fee + (rewards - fee) * margin)`. | ||
| // | ||
| // This is not strictly correct, as the operator share has much more involved | ||
| // formula. This method is a workaround to make the data compatible with the | ||
| // blockfrost dbsync version. | ||
| fn bf_compatible_fees(log: &StakeLog) -> u64 { | ||
| let margin = log | ||
| .margin_cost | ||
| .as_ref() | ||
| .map( | ||
| |pallas::ledger::primitives::RationalNumber { | ||
| numerator, | ||
| denominator, | ||
| }| num_rational::Rational64::new(*numerator as i64, *denominator as i64), | ||
| ) | ||
| .unwrap_or(num_rational::Rational64::from_integer(0)); | ||
|
|
||
| let rewards = num_rational::Rational64::from_integer(log.total_rewards as i64); | ||
| let fixed_cost = num_rational::Rational64::from_integer(log.fixed_cost as i64); | ||
|
|
||
| let variable_fees = (rewards - fixed_cost) * margin; | ||
| let fixed_fees = fixed_cost; | ||
|
|
||
| let fees = variable_fees + fixed_fees; | ||
| let fees = fees.to_integer() as u64; | ||
|
|
||
| if fees > log.total_rewards { | ||
| log.total_rewards | ||
| } else { | ||
| fees | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify truncation behavior matches FLOOR semantics.
The implementation uses to_integer() (line 229) which truncates towards zero, while the comment on line 205 mentions FLOOR. For positive values, these are equivalent, but they differ for negative values:
FLOOR(-1.5) = -2to_integer(-1.5) = -1
Given that pool fees should be non-negative in practice and the cap at total_rewards (lines 231-235) provides a safeguard, this is likely acceptable. However, verify that:
- The blockfrost dbsync formula truly uses
FLOORor if it also truncates - Edge cases where
fixed_cost > rewardsproduce compatible results
The cap preventing fees from exceeding total_rewards is a good safeguard.
🌐 Web query:
Does PostgreSQL FLOOR function and Rust Rational64 to_integer produce different results for negative numbers?
💡 Result:
Yes — they differ for negative values.
- PostgreSQL FLOOR returns the largest integer <= x (rounds toward −∞). Example: FLOOR(-3.14) = -4. [1]
- num-rational's Rational64::to_integer converts by truncation (rounds toward 0). Example: Ratio(-3,2).to_integer() = -1. [2]
So for negative non-integers FLOOR gives a more negative result than Rational64::to_integer. [1][2]
Sources:
[1] PostgreSQL FLOOR behavior (examples showing FLOOR(-32.65) → -33).
[2] num-rational Ratio::to_integer doc (described as converting to integer by rounding toward zero).
Implement proper FLOOR semantics or add explicit safeguard against negative fees.
PostgreSQL's FLOOR function rounds toward −∞ (e.g., FLOOR(-3.14) = -4), while Rust's Rational64::to_integer() truncates toward 0. The code uses to_integer() at line 229, which does not match the FLOOR behavior documented in the comment at line 205.
More critically, when fixed_cost > rewards, the intermediate calculation (rewards - fixed_cost) * margin becomes negative. If this outweighs fixed_cost, the result can be negative. The cap at lines 231-235 only prevents fees from exceeding total_rewards—it does not guard against negative fees, which violates the blockfrost dbsync formula semantics.
Fix: Either replace to_integer() with proper FLOOR logic (e.g., fees.floor().to_integer()), or add explicit handling to ensure fees remain non-negative.
🤖 Prompt for AI Agents
In crates/minibf/src/routes/pools.rs around lines 204 to 236, the fee
computation uses Rational64::to_integer() which truncates toward zero and can
produce incorrect values vs PostgreSQL FLOOR semantics and can yield negative
fees when fixed_cost > rewards; replace the truncation with explicit FLOOR
semantics (call the Rational64 floor operation before converting to integer) or,
if you prefer, clamp the computed fees to a minimum of 0 after flooring, then
cast to u64 and still apply the existing cap against log.total_rewards so the
returned value is always between 0 and total_rewards.
1 participant
Summary by CodeRabbit
New Features
Refactor