Skip to content

Conversation

@Nachiket-Roy
Copy link
Contributor

@Nachiket-Roy Nachiket-Roy commented Nov 19, 2025

Closes #3746
This update introduces a fully automated bounty payout workflow triggered when a pull request is merged.
Key features include:
Automatic detection of bounty labels (e.g., $5, $10)
BCH-based payout processing with secure address validation
Duplicate payment prevention
GitHub PR comment notifications with transaction details
email alerts for missing payment addresses
proper logging and error handling

Summary by CodeRabbit

  • New Features

    • Automated bounty payouts: merged PRs with bounty labels can trigger BCH payments to contributors, add a confirmation comment on the PR, record transactions, and prevent duplicate payments.
    • Notifications when contributors lack a payment address and admin alerts on payment failures; payments respect configurable limits.
  • Chores

    • Added AutoPay environment settings: BCH_API_KEY, BCH_WALLET_ADDRESS, PAYMENT_ENABLED, MAX_AUTO_PAYMENT.

@github-actions
Copy link
Contributor

👋 Hi @Nachiket-Roy!

This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:

  • The PR author
  • DonnieBLT
  • coderabbit
  • copilot

Once a valid peer review is submitted, this check will pass automatically. Thank you!

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

Warning

Rate limit exceeded

@Nachiket-Roy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 23 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7c7d02a and a7a985a.

📒 Files selected for processing (1)
  • website/views/user.py (5 hunks)

Walkthrough

Added AutoPay environment variables, a new send_bch_payment() helper to call an external BCH provider, and an automated bounty payout flow in PR handling that detects bounty labels on merged PRs, converts USD→BCH, attempts payment, records transactions, and posts notifications or failure alerts.

Changes

Cohort / File(s) Summary
Environment Configuration
\.env.example
Added BCH_API_KEY, BCH_WALLET_ADDRESS, PAYMENT_ENABLED, and MAX_AUTO_PAYMENT entries appended after GITHUB_CLIENT_ID.
Payment Execution
website/bitcoin_utils.py
Added send_bch_payment(address, amount) to validate BCH address, format amount to 8 decimals, POST to BCH_PAYMENT_API_URL with BCH_API_KEY, and raise on network/provider/response errors; added Decimal and requests imports.
Bounty Payment Workflow
website/views/user.py
Added bounty/payment helpers and flow: extract_bounty_from_labels, send_crypto_payment (BCH path uses send_bch_payment), process_bounty_payment, record_payment, post_payment_comment, check_existing_payment, notify_user_missing_address, notify_admin_payment_failure; integrated into PR-merged webhook handling to detect bounty labels, prevent duplicates, convert USD→BCH, invoke payment, persist tx, and post PR comments; minor refactor to update_bch_address username defaulting and added imports (re, Decimal, requests, transaction) and Repo import.

Sequence Diagram(s)

sequenceDiagram
    participant GH as GitHub PR
    participant View as views/user.py
    participant Utils as bitcoin_utils.py
    participant API as BCH Provider API
    participant DB as Database
    participant Notify as Notifications

    GH->>View: PR merged event
    View->>View: extract_bounty_from_labels(labels)
    alt bounty label present
        View->>View: check_existing_payment(repo, pr_number)
        alt no existing payment
            View->>View: resolve user BCH address
            alt address present
                View->>View: convert USD→BCH
                View->>Utils: send_bch_payment(address, bch_amount)
                Utils->>API: POST /payment (BCH_API_KEY)
                alt API returns tx_id
                    API-->>Utils: {tx_id}
                    Utils-->>View: tx_id
                    View->>DB: record_payment(pr, tx_id, amounts)
                    View->>GH: post_payment_comment(tx_id, amount)
                else API error
                    API-->>Utils: error
                    Utils-->>View: raise
                    View->>Notify: notify_admin_payment_failure(pr, error)
                end
            else missing address
                View->>Notify: notify_user_missing_address(user, pr)
            end
        else duplicate payment -> View: skip payment
        end
    else no bounty -> View: no-op
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Focus review on:
    • website/bitcoin_utils.py — BCH address validation, Decimal handling, HTTP request construction, error paths, and response parsing.
    • website/views/user.py — label parsing, duplicate-check logic, USD→BCH conversion, atomic transaction handling, recording to Repo/GitHubIssue, and PR comment/notification flows.
    • Configuration gating: PAYMENT_ENABLED, MAX_AUTO_PAYMENT, and secure handling of BCH_API_KEY/BCH_WALLET_ADDRESS.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add Automated Bounty Payment System for Merged Pull Requests' accurately and clearly summarizes the main change, which is the introduction of an automated system for processing bounty payments when PRs are merged.
Linked Issues check ✅ Passed The pull request successfully implements all requirements from issue #3746: automatic bounty label detection, payment triggering on PR merge, BCH payment processing with address validation, duplicate payment prevention, and appropriate notifications.
Out of Scope Changes check ✅ Passed All changes are directly scoped to implementing the automated bounty payment system: environment configuration, BCH payment backend, payment workflow integration, and notification handlers. No unrelated modifications detected.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
website/views/user.py (1)

986-1013: Critical: Do not run autopay while GitHub signature validation is disabled

github_webhook is currently accepting unauthenticated POSTs (@csrf_exempt) and the HMAC signature check is commented out (Lines 986–994). With the new logic:

  • handle_pull_request_event now calls process_bounty_payment whenever it sees action == "closed" and pull_request["merged"] and a $N bounty label on the PR.
  • process_bounty_payment then selects the user’s BCH address and actually initiates a payment via send_crypto_payment.

Without enforcing the GitHub signature (X-Hub-Signature-256) or some equivalent authentication, anyone who can reach this endpoint can forge a payload that:

  • Pretends a PR on any repo was merged.
  • Includes arbitrary labels like "$500" to drive the bounty amount.
  • Spoofs pull_request["user"]["html_url"] to direct funds to any account in your system with a BCH address.

That turns this endpoint into a remotely triggerable money transfer API.

This must be addressed before enabling real payouts:

  • Re‑enable and fix the signature validation so only genuine GitHub webhook events are accepted, and reject requests without a valid signature with 403.
  • Consider failing the entire request with a non‑200 response when validation fails instead of silently proceeding.
  • Only call process_bounty_payment after signature verification passes.

Until that is in place, I’d strongly recommend gating all payment behavior behind a hard settings.PAYMENT_ENABLED check that defaults to False in production.

Also applies to: 1018-1033, 1159-1214

🧹 Nitpick comments (3)
.env.example (1)

46-50: Clarify AutoPay env semantics, defaults, and quoting

Right now the example leaves some important things ambiguous and slightly unsafe by default:

  • MAX_AUTO_PAYMENT=50 doesn’t specify units (USD? BCH? per‑PR cap?), but the code treats bounty amounts as crypto; this should be explicitly documented to avoid misconfiguration.
  • PAYMENT_ENABLED is shown as true in the example; given this controls real payouts, it’s safer if the example uses a disabled default (false) so new deployments don’t accidentally enable autopay.
  • send_bch_payment uses settings.BCH_PAYMENT_API_URL, but this variable isn’t documented here; it should be added with a placeholder so the example is complete.
  • To satisfy dotenv-linter and avoid surprises when values contain special characters, consider quoting these values, e.g.:
    PAYMENT_ENABLED="false"
    MAX_AUTO_PAYMENT="50"
    BCH_API_KEY="your_payment_provider_api_key"
    BCH_WALLET_ADDRESS="your_sponsor_wallet_address"
    BCH_PAYMENT_API_URL="https://example-bch-provider/api/payments"

Also consider adding a trailing newline for linting cleanliness, but that’s cosmetic.

website/bitcoin_utils.py (1)

64-110: Tighten error handling and exception types in send_bch_payment

The happy‑path logic (basic address validation, 8‑decimal formatting, provider call, tx id check) looks reasonable, but the error path can be improved:

  • You currently raise generic Exception in several places and lose the original exception context:
    • Network/provider errors (RequestException) are re‑raised as Exception("BCH network/payment provider unreachable").
    • Non‑200 responses, provider “error” fields, and missing transaction_id also raise generic Exception.
  • This makes it harder to distinguish between transient network issues, provider errors, and coding/config errors when debugging or reacting upstream.

Consider introducing a dedicated exception type (e.g., BchPaymentError) and chaining the original exception where applicable:

class BchPaymentError(Exception):
    pass

def send_bch_payment(address, amount):
    ...
    try:
        response = requests.post(url, json=payload, headers=headers, timeout=10)
    except requests.exceptions.RequestException as exc:
        logger.exception("BCH payment request failed")
        raise BchPaymentError("BCH network/payment provider unreachable") from exc

    if response.status_code != 200:
        logger.error("BCH payment failed (%s): %s", response.status_code, response.text)
        raise BchPaymentError(f"BCH payment failed: {response.text}")

    try:
        data = response.json()
    except ValueError as exc:
        logger.exception("Invalid JSON in BCH payment response")
        raise BchPaymentError("Invalid BCH payment response (non‑JSON)") from exc
    ...

This keeps logs richer (logger.exception) and lets callers handle BchPaymentError explicitly without conflating it with unrelated exceptions.

website/views/user.py (1)

1211-1213: Improve exception logging and granularity in process_bounty_payment

Catching a broad Exception makes sense here to avoid crashing webhook handling on payment/provider issues, but you currently:

  • Log with logger.error(f"Payment failed for PR #{pr_number}: {str(e)}"), which loses the stack trace.
  • Then call notify_admin_payment_failure, which itself can raise (e.g., mail backend issues) and isn’t wrapped.

Consider:

  • Using logger.exception("Payment failed for PR #%s", pr_number) so the stack trace is preserved.
  • Narrowing the except to the types you actually expect (BchPaymentError, requests.exceptions.RequestException, ValueError for bad addresses, etc.) once send_bch_payment exposes a dedicated exception, so unrelated bugs don’t silently masquerade as “payment failure”.
  • Optionally wrapping notify_admin_payment_failure in a small try/except inside this block so email failures don’t cause a 500 on the webhook.

These changes won’t alter the core behavior but will make operational debugging much easier.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 0440557 and ecb382f.

📒 Files selected for processing (3)
  • .env.example (1 hunks)
  • website/bitcoin_utils.py (2 hunks)
  • website/views/user.py (3 hunks)
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
.env.example

[warning] 50-50: [EndingBlankLine] No blank line at the end of the file

(EndingBlankLine)


[warning] 50-50: [UnorderedKey] The MAX_AUTO_PAYMENT key should go before the PAYMENT_ENABLED key

(UnorderedKey)


[warning] 50-50: [ValueWithoutQuotes] This value needs to be surrounded in quotes

(ValueWithoutQuotes)

🪛 Ruff (0.14.5)
website/bitcoin_utils.py

73-73: Avoid specifying long messages outside the exception class

(TRY003)


87-87: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


87-87: Use explicit conversion flag

Replace with conversion flag

(RUF010)


88-88: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


88-88: Create your own exception

(TRY002)


88-88: Avoid specifying long messages outside the exception class

(TRY003)


93-93: Create your own exception

(TRY002)


93-93: Avoid specifying long messages outside the exception class

(TRY003)


100-100: Create your own exception

(TRY002)


100-100: Avoid specifying long messages outside the exception class

(TRY003)


106-106: Create your own exception

(TRY002)


106-106: Avoid specifying long messages outside the exception class

(TRY003)

website/views/user.py

1070-1070: Unpacked variable created is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)


1211-1211: Do not catch blind exception: Exception

(BLE001)


1212-1212: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


1212-1212: Use explicit conversion flag

Replace with conversion flag

(RUF010)

⏰ 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). (2)
  • GitHub Check: Run Tests
  • GitHub Check: docker-test
🔇 Additional comments (1)
website/views/user.py (1)

4-8: New imports and model usage look appropriate

re, Decimal, requests, and the Repo model are all used in the new bounty/autopay helpers (label parsing, amount handling, GitHub comment posting, and repo lookup). Nothing extraneous here; import placement and usage are consistent with the rest of the module.

Also applies to: 54-54

@github-project-automation github-project-automation bot moved this from Backlog to Ready in 📌 OWASP BLT Project Board Nov 19, 2025
@github-actions github-actions bot added unresolved-conversations: 2 PR has 2 unresolved conversations and removed unresolved-conversations: 0 PR has 0 unresolved conversations labels Nov 19, 2025
@github-actions github-actions bot added unresolved-conversations: 1 PR has 1 unresolved conversation unresolved-conversations: 0 PR has 0 unresolved conversations and removed unresolved-conversations: 2 PR has 2 unresolved conversations unresolved-conversations: 1 PR has 1 unresolved conversation labels Nov 19, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
website/views/user.py (2)

139-140: Remove unnecessary fallback: authenticated users always have usernames.

Line 139 adds a fallback to "default_username", but earlier code (line 121) already accesses request.user.userprofile, which requires authentication. If request.user is not authenticated, the code would fail earlier. Additionally, authenticated Django users always have a username, so the or "default_username" clause is unnecessary and could mask bugs.

Apply this diff:

-        username = request.user.username or "default_username"
+        username = request.user.username
         return redirect(reverse("profile", args=[username]))

1226-1229: Use logging.exception in exception handlers for better debugging.

Lines 1227 and 1283 use logging.error within exception handlers. When logging exceptions, prefer logging.exception, which automatically includes the stack trace, making debugging significantly easier.

Apply this diff:

     except Exception as e:
-        logger.error(f"Unable to fetch BCH price: {e}")
+        logger.exception("Unable to fetch BCH price")
         notify_admin_payment_failure(pr_data, "Unable to fetch BCH/USD rate")
         return

And:

     except Exception as e:
-        logger.error(f"Payment failed for PR #{pr_number}: {str(e)}")
+        logger.exception(f"Payment failed for PR #{pr_number}")
         notify_admin_payment_failure(pr_data, str(e))

Note: logging.exception automatically includes exception details, so you don't need to pass e explicitly.

Also applies to: 1282-1284

.env.example (1)

46-50: Add proper formatting for environment variables.

The new AutoPay variables are correctly defined, but need minor formatting improvements for consistency and best practices:

  1. Missing final newline (line 50): Environment files should end with a newline
  2. Unquoted numeric value (line 50): While 50 works unquoted, quoting it as "50" maintains consistency with other values and prevents shell interpretation issues
  3. Variable ordering (optional): Consider placing PAYMENT_ENABLED before MAX_AUTO_PAYMENT for alphabetical consistency

Apply this diff:

 #AutoPay Bounty
 BCH_API_KEY=your_payment_provider_api_key
 BCH_WALLET_ADDRESS=your_sponsor_wallet_address
-PAYMENT_ENABLED=False
-MAX_AUTO_PAYMENT=50  # Maximum amount for automatic payment
+PAYMENT_ENABLED="False"
+MAX_AUTO_PAYMENT="50"  # Maximum amount for automatic payment (USD)
+

Note: Added clarification that MAX_AUTO_PAYMENT is in USD to match the implementation.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between ecb382f and cde0d39.

📒 Files selected for processing (2)
  • .env.example (1 hunks)
  • website/views/user.py (5 hunks)
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
.env.example

[warning] 50-50: [EndingBlankLine] No blank line at the end of the file

(EndingBlankLine)


[warning] 50-50: [UnorderedKey] The MAX_AUTO_PAYMENT key should go before the PAYMENT_ENABLED key

(UnorderedKey)


[warning] 50-50: [ValueWithoutQuotes] This value needs to be surrounded in quotes

(ValueWithoutQuotes)

🪛 Ruff (0.14.5)
website/views/user.py

1061-1061: Unused function argument: bch_amount

(ARG001)


1070-1070: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


1226-1226: Do not catch blind exception: Exception

(BLE001)


1227-1227: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


1282-1282: Do not catch blind exception: Exception

(BLE001)


1283-1283: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


1283-1283: Use explicit conversion flag

Replace with conversion flag

(RUF010)

⏰ 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). (2)
  • GitHub Check: Run Tests
  • GitHub Check: docker-test
🔇 Additional comments (10)
website/views/user.py (10)

4-4: LGTM: New imports support the bounty payment functionality.

All new imports are properly utilized in the payment workflow (regex for label parsing, Decimal for precise currency calculations, requests for API calls, transaction for atomic DB operations, and Repo for repository tracking).

Also applies to: 6-6, 8-8, 19-19, 54-54


1017-1032: LGTM: Clean integration of bounty detection into PR merge workflow.

The enhancement properly detects merged PRs, extracts bounty amounts from labels, and triggers the payment flow only when bounty labels are present. The flow integrates smoothly with the existing badge assignment logic.


1036-1042: LGTM: Robust label parsing with precise decimal handling.

The regex pattern correctly matches bounty labels (e.g., $5, $10.50) and uses Decimal for precise currency arithmetic, avoiding floating-point errors.


1045-1058: LGTM: Clean abstraction for payment provider integration.

The function provides a clear interface for cryptocurrency payments. The conditional import of send_bch_payment is appropriate, and the NotImplementedError for unsupported currencies makes future expansion straightforward.


1123-1151: LGTM: Well-structured PR comment with transaction details.

The function properly formats and posts payment confirmation comments to GitHub PRs. Good practices include:

  • Request timeout to prevent hanging (line 1150)
  • Blockchain explorer link for transparency (line 1136)
  • Proper error handling via status code check (line 1151)

1154-1167: LGTM: Clear email notification for missing payment addresses.

The email provides actionable instructions for users to add their BCH address, with proper settings references for URL and sender email.


1170-1181: LGTM: Proper repo allow-listing now prevents payments to untracked repositories.

The function now correctly returns True (treat as ineligible) when a repo is not tracked (lines 1173-1175), addressing the security concern from previous reviews. This ensures payments are only made for explicitly tracked repositories.


1199-1215: Excellent: Safety gates now properly implemented!

The addition of PAYMENT_ENABLED and MAX_AUTO_PAYMENT checks addresses critical concerns from previous reviews. The implementation correctly:

  • Early-returns when payments are disabled (lines 1200-1202)
  • Validates the MAX_AUTO_PAYMENT setting and converts it to Decimal (line 1207)
  • Compares USD amounts against the limit before proceeding (lines 1208-1215)

This prevents accidental payments in non-production environments and caps automatic payouts to protect against excessive costs.


1217-1229: Excellent: Currency conversion from USD to BCH now implemented!

The code now properly addresses the critical unit mismatch issue raised in previous reviews. Instead of sending $5 as literal 5 BCH, the flow:

  1. Fetches real-time BCH/USD exchange rate from CoinGecko (lines 1218-1223)
  2. Converts USD bounty amount to BCH: bch_amount = usd_amount / rate (line 1225)
  3. Handles API failures gracefully with admin notification (lines 1226-1229)

This ensures bounty labels like $5 are correctly interpreted as $5 USD worth of BCH, not 5 BCH.


1231-1285: LGTM: Comprehensive payment processing with proper validation and error handling.

The payment flow is well-structured:

  • ✅ Address validation (lines 1240-1244)
  • ✅ Duplicate payment prevention (lines 1246-1252)
  • ✅ Detailed logging at each step (lines 1256-1260, 1277-1280)
  • ✅ Transaction recording and PR commenting (lines 1265-1275)
  • ✅ Error handling with admin notifications (lines 1282-1284)

The implementation follows security best practices and handles edge cases appropriately.

@github-actions github-actions bot added unresolved-conversations: 1 PR has 1 unresolved conversation and removed unresolved-conversations: 0 PR has 0 unresolved conversations labels Nov 19, 2025
@github-actions github-actions bot added unresolved-conversations: 0 PR has 0 unresolved conversations and removed unresolved-conversations: 1 PR has 1 unresolved conversation labels Nov 19, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
website/views/user.py (1)

1194-1283: Excellent: All previous safety concerns addressed, minor logging improvements needed.

The function now properly implements all safety gates from previous reviews:

  • PAYMENT_ENABLED check (lines 1200-1202)
  • MAX_AUTO_PAYMENT enforcement (lines 1205-1215)
  • ✅ USD to BCH conversion via CoinGecko API (lines 1217-1229), fixing the unit mismatch issue
  • ✅ Repo allowlisting via check_existing_payment (lines 1246-1252)

However, logging in exception handlers should use logger.exception() to capture stack traces, and the f-string should use an explicit conversion flag.

Apply this diff to improve error logging:

     except Exception as e:
-        logger.error(f"Unable to fetch BCH price: {e}")
+        logger.exception("Unable to fetch BCH price")
         notify_admin_payment_failure(pr_data, "Unable to fetch BCH/USD rate")
         return

     # Step 1: Select BCH address
     except Exception as e:
-        logger.error(f"Payment failed for PR #{pr_number}: {str(e)}")
+        logger.exception(f"Payment failed for PR #{pr_number}: {e!s}")
         notify_admin_payment_failure(pr_data, str(e))
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between cde0d39 and 6e1fffe.

📒 Files selected for processing (1)
  • website/views/user.py (5 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
website/views/user.py

1099-1099: Undefined name created

(F821)


1226-1226: Do not catch blind exception: Exception

(BLE001)


1227-1227: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


1281-1281: Do not catch blind exception: Exception

(BLE001)


1282-1282: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


1282-1282: Use explicit conversion flag

Replace with conversion flag

(RUF010)

⏰ 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). (2)
  • GitHub Check: Run Tests
  • GitHub Check: docker-test
🔇 Additional comments (9)
website/views/user.py (9)

4-4: LGTM: Necessary imports for bounty payment system.

The new imports support the payment automation features: re for label parsing, Decimal for precise currency calculations, requests for external API calls, transaction for atomic database operations, and Repo for repository tracking.

Also applies to: 6-6, 8-8, 19-19, 54-54


139-139: LGTM: Pythonic improvement.

The change from conditional expression to or operator is more idiomatic while maintaining the same fallback behavior for empty usernames.


1017-1033: LGTM: Clean integration of bounty payment trigger.

The enhancement correctly detects merged PRs with bounty labels and triggers automated payment processing while preserving existing badge assignment functionality.


1036-1042: LGTM: Robust bounty label parsing.

The regex pattern correctly matches dollar-amount labels (e.g., $5, $10.50), and using Decimal ensures precise currency handling. Returning None for missing bounty labels is appropriate.


1045-1058: LGTM: Clean payment delegation.

The function properly delegates to the BCH payment implementation and appropriately raises NotImplementedError for unsupported payment methods, making future extension points clear.


1123-1151: LGTM: Well-structured GitHub comment notification.

The function properly handles flexible amount formats, includes a blockchain explorer link for transparency, and uses appropriate timeout for the GitHub API call.


1154-1167: LGTM: Clear and actionable user notification.

The email notification provides comprehensive instructions, including the specific BCH address format requirement, enabling users to quickly add payment details.


1170-1181: LGTM: Proper repo allowlisting and duplicate detection.

The function correctly treats untracked repositories as ineligible for payment (returning True to skip processing) and reliably detects duplicate payments via transaction IDs. This addresses the previous review concern about incomplete repo allowlisting.


1184-1191: LGTM: Essential admin error notification.

The function provides admins with critical payment failure information for debugging and intervention.

@github-actions github-actions bot added unresolved-conversations: 1 PR has 1 unresolved conversation and removed unresolved-conversations: 0 PR has 0 unresolved conversations labels Nov 19, 2025
@github-actions github-actions bot removed the unresolved-conversations: 1 PR has 1 unresolved conversation label Nov 19, 2025
@github-actions github-actions bot added the unresolved-conversations: 0 PR has 0 unresolved conversations label Nov 19, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
website/views/user.py (1)

1204-1215: Add error handling for MAX_AUTO_PAYMENT conversion.

Line 1207 converts MAX_AUTO_PAYMENT to Decimal without error handling. If the setting contains an invalid value, this will raise a ValueError and crash the webhook handler.

Apply this diff to add defensive error handling:

     max_auto = getattr(settings, "MAX_AUTO_PAYMENT", None)
     if max_auto is not None:
-        max_auto_dec = Decimal(str(max_auto))
-        if usd_amount > max_auto_dec:
-            logger.warning(
-                "USD bounty %s exceeds MAX_AUTO_PAYMENT %s; skipping autopay for PR #%s",
-                usd_amount,
-                max_auto_dec,
-                pr_data["number"],
-            )
-            return
+        try:
+            max_auto_dec = Decimal(str(max_auto))
+        except (ValueError, TypeError, ArithmeticError) as e:
+            logger.error("Invalid MAX_AUTO_PAYMENT=%r; skipping autopay: %s", max_auto, e)
+            return
+        if usd_amount > max_auto_dec:
+            logger.warning(
+                "USD bounty %s exceeds MAX_AUTO_PAYMENT %s; skipping autopay for PR #%s",
+                usd_amount,
+                max_auto_dec,
+                pr_data["number"],
+            )
+            return
🧹 Nitpick comments (1)
website/views/user.py (1)

1036-1042: Suggestion: Document behavior when multiple bounty labels exist.

The function returns the first matching bounty label. If multiple labels like $5 and $10 are present, only the first is processed. Consider documenting this behavior or adding validation to prevent multiple bounty labels.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 6e1fffe and 7c7d02a.

📒 Files selected for processing (1)
  • website/views/user.py (5 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
website/views/user.py

1226-1226: Do not catch blind exception: Exception

(BLE001)


1227-1227: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


1281-1281: Do not catch blind exception: Exception

(BLE001)


1282-1282: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


1282-1282: Use explicit conversion flag

Replace with conversion flag

(RUF010)

⏰ 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). (2)
  • GitHub Check: Run Tests
  • GitHub Check: docker-test
🔇 Additional comments (5)
website/views/user.py (5)

1017-1033: LGTM! Bounty detection and payment triggering implemented correctly.

The enhancement properly integrates bounty payment into the PR merge workflow, with appropriate safety gates handled in process_bounty_payment.


1045-1058: LGTM! Clean payment abstraction.

The function provides a good abstraction layer for future payment methods while currently supporting BCH.


1061-1121: LGTM! Payment recording is robust.

The function properly:

  • Validates repo tracking before recording
  • Uses atomic transactions to prevent partial writes
  • Handles Decimal arithmetic safely
  • Logs exceptions with stack traces

All issues from previous reviews have been addressed.


1123-1151: LGTM! PR comment notification implemented correctly.

The function posts clear payment confirmation with transaction details. The timeout parameter prevents hanging on slow responses.


1154-1191: LGTM! Notification helpers are well-implemented.

The helper functions properly handle:

  • User notifications for missing payment addresses
  • Duplicate payment prevention with correct repo allowlisting
  • Admin alerts for payment failures

@github-actions github-actions bot added unresolved-conversations: 1 PR has 1 unresolved conversation and removed unresolved-conversations: 0 PR has 0 unresolved conversations labels Nov 19, 2025
@github-actions github-actions bot added unresolved-conversations: 0 PR has 0 unresolved conversations and removed unresolved-conversations: 1 PR has 1 unresolved conversation labels Nov 19, 2025
@sidd190
Copy link
Contributor

sidd190 commented Nov 19, 2025

Ahhh, this seems to have a few problems.

  • Some critical ones, maybe we should use HMAC signature verification for Github Webhook, maybe re-enable the github default verification for payment related requests.
  • Consider adding a check for only the whitelisted repo to gate payment processing to only selected repositories.
  • Also consider adding rate-limiting, volume or replay checks and code to prevent race-conditions coz it can be critical in payment related changes.
  • Maybe add code to prevent idempotency, I couldn't find any or maybe I missed, but if not implemented, use a db transaction to wrap the entire payment + DB + comment preferrably to avoid double payments.

I think a more security critical review is required on this, maybe ping @DonnieBLT as well so he can take a look and tell you better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

files-changed: 3 PR changes 3 files unresolved-conversations: 0 PR has 0 unresolved conversations

Projects

Status: Ready

2 participants