feat(billing): add miner revenue payment tracking #299
Conversation
- Add paid/tx_hash columns to miner_revenue_summary table - Add GetUnpaidMinerRevenueSummary gRPC endpoint for querying unpaid records - Add MarkMinerRevenuePaid gRPC endpoint for recording payments - Add pay_miners.py script for automated TAO/Alpha payments via Bittensor
WalkthroughAdds payment-tracking to miner revenue: DB migrations add Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as pay_miners.py (CLI)
participant BillingClient
participant BillingSvc as gRPC BillingService
participant Storage as DB/Repository
participant Processor as MinerPaymentProcessor
participant Wallet as Wallet/Node
participant Chain as Blockchain
User->>CLI: run pay_miners
CLI->>BillingClient: get_all_unpaid_summaries()
BillingClient->>BillingSvc: GetUnpaidMinerRevenueSummary RPC
BillingSvc->>Storage: get_unpaid_summaries(filter)
Storage-->>BillingSvc: summaries
BillingSvc-->>BillingClient: response
BillingClient-->>CLI: summaries
CLI->>Processor: calculate_payments(summaries)
Processor->>Processor: fetch_tao_price(CoinGecko)
Processor-->>CLI: PaymentSummary
CLI->>User: show summary & request approval
User-->>CLI: approve
CLI->>Processor: execute_payments(summary)
loop per payment
Processor->>Wallet: unlock & prepare
Processor->>Chain: transfer TAO/Alpha
Chain-->>Processor: tx_hash
Processor->>BillingClient: mark_as_paid(id, tx_hash)
BillingClient->>BillingSvc: MarkMinerRevenuePaid RPC
BillingSvc->>Storage: mark_summary_paid(id, tx_hash)
Storage-->>BillingSvc: success/fail
BillingSvc-->>BillingClient: response
BillingClient-->>Processor: confirmation
end
Processor-->>CLI: results
CLI-->>User: final report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
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 |
itzlambda
force-pushed
the
miner-revenue-summary-exploration
branch
2 times, most recently
from
ef07807 to
63e5b3a
Compare
itzlambda
force-pushed
the
miner-revenue-summary-exploration
branch
from
63e5b3a to
f9cc019
Compare
Alpha payments were incorrectly passing the converted alpha amount to add_stake(), which expects TAO. This caused overpayment by the alpha/TAO exchange rate.
Updates documentation and comments to accurately reflect that total_revenue stores USD values, not TAO. Also updates bittensor SDK usage to match v10 API changes.
Replace silent warnings with immediate script termination when mark_as_paid fails after a successful blockchain transaction.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
crates/basilica-billing/migrations/036_miner_revenue_payment_tracking.sql (1)
6-8: Consider adding a unique constraint ontx_hash.A unique constraint on
tx_hashwould prevent accidentally marking multiple revenue summaries with the same transaction hash, which could indicate a double-payment bug.However, if a single blockchain transaction can legitimately pay multiple summaries (batch payments), this constraint would be inappropriate. Please verify the intended payment model.
crates/basilica-billing/src/domain/miner_revenue.rs (2)
182-221: Consider extracting shared validation logic.The period and pagination validation (lines 186-206) is duplicated from
get_summaries. A helper method could reduce duplication:fn validate_pagination(limit: Option<i64>, offset: Option<i64>) -> Result<()> { ... }This is a minor refactor that can be addressed later.
223-228: Consider validatingtx_hashformat.The current validation only checks for empty strings. Consider adding basic format validation for blockchain transaction hashes (e.g., minimum length, valid characters) to catch malformed inputs early.
This could prevent accidental storage of garbage data like whitespace-only strings or truncated hashes.
🔎 Example validation
async fn mark_as_paid(&self, id: Uuid, tx_hash: &str) -> Result<bool> { - if tx_hash.is_empty() { + let tx_hash = tx_hash.trim(); + if tx_hash.is_empty() || tx_hash.len() < 10 { return Err(BillingError::InvalidState { - message: "Transaction hash cannot be empty".to_string(), + message: "Transaction hash is invalid or too short".to_string(), }); }crates/basilica-protocol/proto/billing.proto (1)
352-357: Consider usinggoogle.protobuf.Timestampfor consistency.The
period_startandperiod_endfields use string dates (YYYY-MM-DD), whileGetMinerRevenueSummaryRequestusesgoogle.protobuf.Timestampfor similar fields (lines 297-298). This inconsistency could cause confusion for API consumers.However, if this matches the existing pattern in
RefreshMinerRevenueSummaryRequest, maintaining consistency with that endpoint may be intentional.crates/basilica-billing/src/grpc/billing_service.rs (1)
1341-1397: Code duplication withget_miner_revenue_summaryresponse mapping.The
MinerRevenueSummaryto proto conversion (lines 1342-1383) duplicates lines 1224-1264. Consider extracting a helper method.🔎 Suggested helper extraction
impl BillingServiceImpl { fn domain_summary_to_proto(s: crate::storage::MinerRevenueSummary) -> basilica_protocol::billing::MinerRevenueSummary { basilica_protocol::billing::MinerRevenueSummary { id: s.id.to_string(), node_id: s.node_id, validator_id: s.validator_id.unwrap_or_default(), miner_uid: s.miner_uid.unwrap_or(0) as u32, miner_hotkey: s.miner_hotkey, period_start: Some(prost_types::Timestamp { seconds: s.period_start.timestamp(), nanos: s.period_start.timestamp_subsec_nanos() as i32, }), period_end: Some(prost_types::Timestamp { seconds: s.period_end.timestamp(), nanos: s.period_end.timestamp_subsec_nanos() as i32, }), total_rentals: s.total_rentals as u32, completed_rentals: s.completed_rentals as u32, failed_rentals: s.failed_rentals as u32, total_revenue: Self::format_decimal(s.total_revenue), total_hours: Self::format_decimal(s.total_hours), avg_hourly_rate: s.avg_hourly_rate.map(Self::format_decimal).unwrap_or_default(), avg_rental_duration_hours: s.avg_rental_duration_hours.map(Self::format_decimal).unwrap_or_default(), computed_at: Some(prost_types::Timestamp { seconds: s.computed_at.timestamp(), nanos: s.computed_at.timestamp_subsec_nanos() as i32, }), computation_version: s.computation_version as u32, created_at: Some(prost_types::Timestamp { seconds: s.created_at.timestamp(), nanos: s.created_at.timestamp_subsec_nanos() as i32, }), paid: s.paid, tx_hash: s.tx_hash.unwrap_or_default(), } } }crates/basilica-billing/src/storage/miner_revenue.rs (1)
508-510: Minor:param_idxnot incremented afterperiod_endbinding.The
param_idxvariable is incremented afterperiod_start(line 505) but not afterperiod_end(line 509). This works correctly becauseperiod_endis the last condition, but it's inconsistent. The same pattern exists incount_unpaid_summaries(lines 558-565).🔎 Suggested consistency fix
if filter.period_end.is_some() { query.push_str(&format!(" AND period_end <= ${}", param_idx)); + param_idx += 1; } + let _ = param_idx; // Mark as intentionally unused after this pointscripts/pay_miners.py (4)
106-106: Remove extraneousfprefixes from strings without placeholders.Several strings use f-string syntax but contain no placeholders.
🔎 Fix locations
- console.print(f"\n[bold]Payment Context:[/bold]") + console.print("\n[bold]Payment Context:[/bold]") - console.print(f"\n[bold yellow]PAYMENT WAS SUCCESSFUL:[/bold yellow]") + console.print("\n[bold yellow]PAYMENT WAS SUCCESSFUL:[/bold yellow]") - console.print(f"\n[bold]DATABASE RECORD NOT UPDATED:[/bold]") + console.print("\n[bold]DATABASE RECORD NOT UPDATED:[/bold]") - console.print(f"\n[bold red]DO NOT RERUN THIS SCRIPT WITHOUT MANUAL INTERVENTION![/bold red]") + console.print("\n[bold red]DO NOT RERUN THIS SCRIPT WITHOUT MANUAL INTERVENTION![/bold red]") - console.print(f"\n[bold]Total Payments:[/bold]") + console.print("\n[bold]Total Payments:[/bold]") - console.print(f"\n[bold]Pricing:[/bold]") + console.print("\n[bold]Pricing:[/bold]") - self.console.print(f" [green]Marked as paid in billing[/green]") + self.console.print(" [green]Marked as paid in billing[/green]")Also applies to: 247-247, 252-252, 256-256, 674-674, 689-689, 819-819
419-426: Useraise ... fromto preserve exception chain.When re-raising exceptions, use
raise ... from original_errororraise ... from Noneto properly chain exceptions and provide better tracebacks.🔎 Example fix pattern
except json.JSONDecodeError as e: raise BillingServiceError( message=f"Invalid JSON response from billing service: {result.stdout[:200]}...", operation="get_unpaid_summaries", original_error=e, - ) + ) from eApply this pattern to all similar locations where custom exceptions are raised from caught exceptions.
Also applies to: 500-507, 543-547, 553-557, 725-731, 737-742, 786-791, 821-828
961-963: Early exit with sys.exit(0) bypasses normal return flow.Using
sys.exit(0)inside_run_payment_flowwhen no summaries are found is acceptable for a CLI script, but it prevents the function from being reused in other contexts (e.g., testing). Consider raising a specific exception or returning early with a flag instead if testability is desired.
636-640: Catching bareExceptionmasks important errors; consider catching specific exceptions from bittensor operations.The code calls multiple bittensor library methods (
Subtensor(),subnet(),subnet.tao_to_alpha()) that may raise specific exceptions. The suggested exceptions (ValueError, AttributeError, TypeError) handle conversion errors, but the bittensor library defines specific exceptions like ChainError, ChainConnectionError, and ChainTransactionError that could occur during subnet queries. Either catch specific bittensor exceptions explicitly, or document why a broad exception catch is necessary for graceful fallback behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/basilica-protocol/src/gen/basilica.billing.v1.rsis excluded by!**/gen/**
📒 Files selected for processing (8)
crates/basilica-billing/migrations/036_miner_revenue_payment_tracking.sql(1 hunks)crates/basilica-billing/migrations/037_fix_total_revenue_comment.sql(1 hunks)crates/basilica-billing/src/domain/miner_revenue.rs(3 hunks)crates/basilica-billing/src/grpc/billing_service.rs(3 hunks)crates/basilica-billing/src/storage/miner_revenue.rs(7 hunks)crates/basilica-billing/src/storage/mod.rs(1 hunks)crates/basilica-protocol/proto/billing.proto(3 hunks)scripts/pay_miners.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
crates/basilica-billing/src/domain/miner_revenue.rs (1)
crates/basilica-billing/src/storage/miner_revenue.rs (2)
get_unpaid_summaries(109-112)get_unpaid_summaries(484-542)
crates/basilica-billing/src/grpc/billing_service.rs (2)
crates/basilica-protocol/src/gen/basilica.billing.v1.rs (6)
get_unpaid_miner_revenue_summary(1038-1067)get_unpaid_miner_revenue_summary(1210-1216)new(687-690)new(1244-1246)mark_miner_revenue_paid(1068-1097)mark_miner_revenue_paid(1217-1223)crates/basilica-billing/src/domain/miner_revenue.rs (1)
new(45-47)
scripts/pay_miners.py (1)
crates/basilica-billing/src/domain/miner_revenue.rs (2)
mark_as_paid(36-36)mark_as_paid(223-245)
🪛 Ruff (0.14.8)
scripts/pay_miners.py
106-106: f-string without any placeholders
Remove extraneous f prefix
(F541)
247-247: f-string without any placeholders
Remove extraneous f prefix
(F541)
252-252: f-string without any placeholders
Remove extraneous f prefix
(F541)
256-256: f-string without any placeholders
Remove extraneous f prefix
(F541)
374-374: Possible hardcoded password assigned to: "token_type"
(S105)
394-394: subprocess call: check for execution of untrusted input
(S603)
395-405: Starting a process with a partial executable path
(S607)
422-426: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
478-478: subprocess call: check for execution of untrusted input
(S603)
479-489: Starting a process with a partial executable path
(S607)
503-507: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
544-547: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
554-557: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
581-581: Possible hardcoded password assigned to: "token_type"
(S105)
635-635: Consider moving this statement to an else block
(TRY300)
636-636: Do not catch blind exception: Exception
(BLE001)
656-656: Possible hardcoded password assigned to: "token_type"
(S105)
674-674: f-string without any placeholders
Remove extraneous f prefix
(F541)
682-682: Possible hardcoded password assigned to: "token_type"
(S105)
689-689: f-string without any placeholders
Remove extraneous f prefix
(F541)
692-692: Possible hardcoded password assigned to: "token_type"
(S105)
725-725: Do not catch blind exception: Exception
(BLE001)
726-731: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
737-737: Do not catch blind exception: Exception
(BLE001)
738-742: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
769-769: Possible hardcoded password assigned to: "token_type"
(S105)
786-786: Do not catch blind exception: Exception
(BLE001)
787-791: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
818-818: Abstract raise to an inner function
(TRY301)
818-818: Avoid specifying long messages outside the exception class
(TRY003)
819-819: f-string without any placeholders
Remove extraneous f prefix
(F541)
821-821: Do not catch blind exception: Exception
(BLE001)
824-828: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
926-926: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (22)
crates/basilica-billing/migrations/037_fix_total_revenue_comment.sql (1)
1-5: LGTM!Good documentation fix. The comment now correctly reflects that
total_revenuestores USD values, which aligns with the proto definition update.crates/basilica-billing/src/storage/mod.rs (1)
24-27: LGTM!The new
UnpaidMinerRevenueSummaryFiltertype is appropriately re-exported alongside related types.crates/basilica-billing/migrations/036_miner_revenue_payment_tracking.sql (1)
10-18: LGTM!Partial indexes are a good choice here.
idx_miner_revenue_summary_unpaidefficiently supports unpaid record queries and will shrink as payments are processed.idx_miner_revenue_summary_tx_hashenables transaction hash lookups for audit purposes.crates/basilica-billing/src/domain/miner_revenue.rs (1)
28-36: LGTM!The new trait methods follow consistent patterns with the existing
get_summariesmethod. Themark_as_paidsignature appropriately returns a boolean to distinguish between success and not-found/already-paid cases.crates/basilica-protocol/proto/billing.proto (3)
28-30: LGTM!The new RPCs follow consistent naming conventions and are logically grouped under miner payment operations.
344-348: LGTM!Field numbers 18-19 appropriately continue the sequence. The payment tracking fields integrate well with the existing message structure.
364-367: LGTM!The request structure is appropriately minimal. The inline comment clarifies that
idis a UUID.crates/basilica-billing/src/grpc/billing_service.rs (3)
1262-1263: LGTM!The
paidandtx_hashfields are correctly added to the existingget_miner_revenue_summaryresponse, maintaining consistency with the new payment tracking fields.
1281-1339: LGTM - Well-structured unpaid summaries endpoint.The implementation correctly:
- Parses optional date filters with proper validation
- Applies sensible pagination defaults (limit=100)
- Reuses the existing filter pattern from
get_miner_revenue_summary- Uses appropriate error handling and logging
1399-1452: LGTM - Robust mark_miner_revenue_paid implementation.Good practices observed:
- Validates both
idandtx_hashare non-empty- Validates UUID format before calling service
- Logs operations for audit trail
- Returns success=false with descriptive message when record not found/already paid (idempotent-friendly)
- Maps errors to response instead of gRPC status (allows clients to handle gracefully)
crates/basilica-billing/src/storage/miner_revenue.rs (6)
40-44: LGTM - Payment tracking fields added correctly.The
paidboolean and optionaltx_hashfields provide the necessary payment state tracking. The positioning after audit fields is logical grouping.
72-79: LGTM - Clean filter struct for unpaid summaries.The filter appropriately limits to period filtering and pagination, avoiding over-engineering.
107-120: LGTM - Well-documented trait extension.The trait methods are clearly documented, especially
mark_summary_paidwhich explains the return value semantics (true = updated, false = already paid or not found).
483-542: LGTM - Correct unpaid summaries query implementation.The query properly:
- Filters by
paid = FALSEas the primary condition- Uses parameterized queries for period filters
- Applies consistent ordering for pagination stability
- Reuses
summary_from_rowfor mapping
589-607: LGTM - Safe conditional update for marking paid.The
WHERE id = $1 AND paid = FALSEcondition ensures:
- Idempotency - re-running with same ID is safe
- No accidental overwrites of already-paid records
- Return value indicates whether update occurred
This is a critical safety feature for the payment workflow.
647-656: LGTM - Test updated for new fields.The test correctly initializes and verifies the new
paidandtx_hashfields.scripts/pay_miners.py (6)
56-126: LGTM - Well-designed error context and hierarchy.The
PaymentContextdataclass captures all necessary information for debugging payment failures. Theformat_for_consolemethod provides clear, actionable error output.
210-272: Critical error handling for double-payment prevention is excellent.The
MarkAsPaidErrorclass correctly identifies this as the most dangerous error state. The recovery instructions (lines 258-267) provide clear manual remediation steps including the exactgrpcurlcommand needed. This is crucial for operational safety.
811-828: Critical safety: mark_as_paid failure handling is correct.This is the most important section of the script. The pattern is correct:
- Payment succeeds →
tx_hashcapturedmark_as_paidcalled withtx_hash- If
mark_as_paidfails →MarkAsPaidErrorraised with full context- Script halts to prevent double-payment on rerun
The explicit check for
mark_result == False(line 817-818) is important - it catches the case where the gRPC call succeeds but the record wasn't updated.
984-988: Good safety check: require --force for multiple periods.This prevents accidental payment of the wrong period when multiple unpaid periods exist. The error message is clear and actionable.
394-408: Subprocess calls use grpcurl from PATH - acceptable for operational scripts.The static analysis flagged
S603(subprocess execution) andS607(partial executable path). For this operational script:
grpcurlis expected to be installed and in PATH- The input data is from the billing service response (not user input)
- The script is meant for trusted operator use
This is acceptable, but consider documenting the
grpcurldependency in the docstring.Also applies to: 478-492
768-805: Response structure is correct for Bittensor SDK v10.ExtrinsicResponse objects returned by
subtensor.transfer()andsubtensor.add_stake()containsuccess,message, andextrinsic_receiptattributes, withextrinsic_receiptholding block information and execution details whenwait_for_inclusion=True. The code's use of these attributes aligns with the documented SDK v10 API. Verify the exact field name for accessing the extrinsic hash withinextrinsic_receiptagainst your SDK version's implementation or type hints.
Change period_start and period_end from google.protobuf.Timestamp to string (YYYY-MM-DD format) for consistency with other miner revenue summary endpoints.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/basilica-protocol/src/gen/basilica.billing.v1.rsis excluded by!**/gen/**
📒 Files selected for processing (2)
crates/basilica-billing/src/grpc/billing_service.rs(4 hunks)crates/basilica-protocol/proto/billing.proto(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/basilica-billing/src/grpc/billing_service.rs (3)
crates/basilica-protocol/src/gen/basilica.billing.v1.rs (6)
get_unpaid_miner_revenue_summary(1038-1067)get_unpaid_miner_revenue_summary(1210-1216)new(687-690)new(1244-1246)mark_miner_revenue_paid(1068-1097)mark_miner_revenue_paid(1217-1223)crates/basilica-billing/src/domain/miner_revenue.rs (1)
new(45-47)crates/basilica-billing/src/storage/miner_revenue.rs (1)
new(128-130)
🔇 Additional comments (6)
crates/basilica-billing/src/grpc/billing_service.rs (2)
23-27: LGTM: Import additions are correct.The new request/response types are properly imported to support the unpaid summary and payment marking endpoints.
1266-1267: LGTM: Payment tracking fields properly mapped.The
paidandtx_hashfields are correctly mapped from the domain model to the protocol message, with appropriate handling of the optionaltx_hashfield.crates/basilica-protocol/proto/billing.proto (4)
28-31: LGTM: New RPCs properly defined.The
GetUnpaidMinerRevenueSummaryandMarkMinerRevenuePaidRPCs are correctly added with appropriate request/response types and clear documentation.
333-333: LGTM: Important clarification of revenue units.The comment update clarifies that
total_revenueis in USD rather than TAO, which is important information for API consumers.
345-348: LGTM: Payment tracking fields properly added.The
paidandtx_hashfields are correctly added with appropriate types and field numbers. Adding new fields to existing messages is backward-compatible in protobuf.
350-372: LGTM: New message types are well-defined.The new message types for fetching unpaid summaries and marking records as paid are properly structured with appropriate field types, clear comments, and consistent pagination patterns.
Consolidates duplicate date parsing and summary-to-proto conversion logic into reusable helpers. Adds validation to ensure period_start <= period_end.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/basilica-billing/src/grpc/billing_service.rs (1)
1383-1436: Idempotency concern remains unaddressed.The endpoint returns
success: falsewith message "Record not found or already paid" whenmark_as_paidreturnsOk(false)(lines 1420-1425). This conflates two distinct scenarios, making idempotent retries problematic. If a client retries marking an already-paid record (possibly with the sametx_hash), it receives a failure, which may trigger additional retries or alerts.For robust operation, consider:
- Treating "already paid with the same tx_hash" as success
- Distinguishing "not found" from "already paid" in error messages
- Only failing for genuinely conflicting cases (e.g., already paid with different tx_hash)
This concern was previously raised and remains unresolved.
🧹 Nitpick comments (1)
crates/basilica-billing/src/grpc/billing_service.rs (1)
194-217: Consider simplifying the return type.The function boxes the
Statuserror only to immediately unbox it at each call site (lines 1242, 1248, 1318, 1324 usemap_err(|e| *e)). This adds unnecessary heap allocation and dereferencing without any benefit.🔎 Proposed simplification
- fn parse_period_date( - date_str: &str, - field_name: &str, - start_of_day: bool, - ) -> Result<chrono::DateTime<chrono::Utc>, Box<Status>> { + fn parse_period_date( + date_str: &str, + field_name: &str, + start_of_day: bool, + ) -> Result<chrono::DateTime<chrono::Utc>, Status> { let date = chrono::NaiveDate::parse_from_str(date_str, "%Y-%m-%d").map_err(|_| { - Box::new(Status::invalid_argument(format!( + Status::invalid_argument(format!( "{} must be in YYYY-MM-DD format", field_name - ))) + )) })?; let dt = if start_of_day { date.and_hms_opt(0, 0, 0).expect("valid time") } else { date.and_hms_micro_opt(23, 59, 59, 999_999) .expect("valid time") }; Ok(dt.and_utc()) }Then update call sites to remove the unboxing:
if !req.period_start.is_empty() { filter.period_start = Some( - Self::parse_period_date(&req.period_start, "period_start", true).map_err(|e| *e)?, + Self::parse_period_date(&req.period_start, "period_start", true)?, ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/basilica-billing/src/grpc/billing_service.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/basilica-billing/src/grpc/billing_service.rs (2)
crates/basilica-billing/src/domain/miner_revenue.rs (1)
new(45-47)crates/basilica-billing/src/storage/miner_revenue.rs (1)
new(128-130)
🔇 Additional comments (3)
crates/basilica-billing/src/grpc/billing_service.rs (3)
126-168: LGTM! Helper function successfully eliminates duplication.The
domain_summary_to_protohelper correctly consolidates the domain-to-proto mapping logic that was previously duplicated. The implementation properly handles Option types, decimal formatting, and timestamp conversions. This addresses the refactoring suggestion from previous reviews.
1240-1250: LGTM! Properly uses the extracted helpers.The endpoint correctly leverages both
parse_period_dateanddomain_summary_to_protohelpers, successfully addressing the code duplication flagged in previous reviews.Also applies to: 1287-1290
1306-1381: LGTM! Endpoint correctly implements unpaid summaries retrieval.The implementation properly validates date ranges (lines 1328-1335), applies pagination with sensible defaults, and leverages the shared helper functions. This addresses the validation concern from previous reviews.
Summary
paidandtx_hashcolumnsGetUnpaidMinerRevenueSummaryandMarkMinerRevenuePaidSummary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.