fix(airdrop): scope _determine_tier to RustChain contributions only - #8187
fix(airdrop): scope _determine_tier to RustChain contributions only#8187rebel117 wants to merge 2 commits into
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
FlintLeng
left a comment
There was a problem hiding this comment.
PR Review: Airdrop Tier Scoping + Bridge Lock Admin Auth
Reviewed on: 2026-08-07
Summary
Two related fixes in airdrop_v2.py:
_determine_tier()now counts only RustChain-org merged PRs instead of all GitHub commits (fixes #8184)- Bridge lock creation gated behind admin key authentication; public lock data stripped via
to_public_dict()
Fix 1: Airdrop Tier Scope ✅
The vulnerability was real. The old code used /search/commits with author:X merged:true — this returns commits authored by the user across all of GitHub, not just merged PRs in the RustChain org. Any established GitHub account would clear the CORE tier (200 wRTC) without a single RustChain contribution.
The fix is correct and well-scoped:
- Switches to
/search/issueswithorg:Scottcjn is:pr is:merged— the correct API for counting merged PRs in a specific org - Removes the
application/vnd.github.cloak-previewAccept header (only needed for commit search) - Removes bare
merged:true(a PR-search qualifier misused on the commits endpoint)
The 7 unit tests are exemplary. They parse the AST of airdrop_v2.py and assert on the actual code structure (not just runtime behaviour), which is the right approach for API-usage validation tests. Tests for: correct endpoint, absent old endpoint, org scope, PR+merged qualifiers, absent bare merged:true, absent cloak header, module compiles.
Fix 2: Bridge Lock Admin Auth ✅
create_bridge_lock gated behind admin key — correct. Creating a bridge lock is a write operation that affects the cross-chain minting supply. Only admins should be able to trigger it.
to_public_dict() data stripping — get_bridge_lock now returns a sanitised view without admin-only fields when no admin key is provided. The has_admin_key() helper checks both X-Admin-Key and X-API-Key headers with hmac.compare_digest (timing-safe comparison) — correct.
Minor note: The admin key env var is RC_ADMIN_KEY. Worth confirming this is documented and set in production deployment, otherwise require_admin_key() will always return None (no auth) when the env var is absent.
Wallet: RTC019e78d600fb3131c29d7ba80aba8fe644be426e
✅ LGTM — both fixes are clean, well-tested, and address real vulnerabilities.
Problem
_determine_tier()innode/airdrop_v2.pywas using/search/commitswith a bareauthor:{username} merged:truequery to decide airdrop eligibility tiers. Two issues with that:No org/repo scope —
total_countreflects commits anywhere on GitHub, not contributions to RustChain. So any account with a normal open-source history immediately qualifies for the CORE tier (5+, 200 wRTC) without ever contributing here.Wrong endpoint for the qualifier —
merged:trueis a pull-request search qualifier. Sent to/search/commits, it does not mean "merged PRs". As shown in the issue,GET /search/commits?q=author:torvalds+merged:truereturns 73,907 results, which obviously isn't merged RustChain PRs.Fix
Switch from
/search/commitsto/search/issueswithis:pr is:mergedqualifiers, and scope the query toorg:Scottcjnso only actual RustChain contributions are counted:The tier thresholds (1/3/5) stay the same — they just now measure what the docstrings always claimed: merged PRs within this project.
Testing
Added
tests/test_airdrop_tier_github_scope_8184.pywith 7 assertions:/search/issues(not/search/commits)org:Scottcjnis:prandis:mergedqualifiersmerged:truequalifier is gonecloak-previewAccept header is goneAll 7 tests pass.
Closes #8184