Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 2eb1a84 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes refactor the rev-share-cap race algorithm to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR refines the leaderboard calculation for the rev-share-cap award model by migrating from raw Confidence Score: 5/5Safe to merge — all changes are a well-typed refactor with no semantic differences from the prior implementation. No P0 or P1 issues found. The migration from raw bigints to Price objects is consistent end-to-end, currency runtime guards are correct, the non-negativity invariant in subtractPrices is enforced, and the race algorithm's logic is preserved exactly. Tests cover the new helpers' happy paths and all throwing conditions. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[sortedEvents] --> B[for each event]
B --> C{referrer state exists?}
C -- No --> D[init state\ntotalRevenueContribution: PriceEth 0\nhasQualified: false\ncappedAward: PriceUsdc 0]
D --> E
C -- Yes --> E[addPrices totalRevenueContribution\n+ event.incrementalRevenueContribution]
E --> F[computeBaseRevenueContribution\nrules x totalIncrementalDuration]
F --> G{isReferrerQualified?}
G -- No --> B
G -- Yes, first time --> H[accumulatedUncappedAward =\nscalePrice totalBaseRevenue\nx maxBaseRevenueShare]
H --> I[incrementalCappedAward =\nminPrices uncapped, awardPoolRemaining]
I --> J[cappedAward += incrementalCappedAward\nawardPoolRemaining -= incrementalCappedAward\nhasQualified = true]
J --> B
G -- Yes, already qualified --> K[incrementalBaseRevenue =\ncomputeBaseRevenueContribution\nevent.incrementalDuration]
K --> L[incrementalUncappedAward =\nscalePrice incremental x maxBaseRevenueShare]
L --> M[incrementalCappedAward =\nminPrices uncapped, awardPoolRemaining]
M --> N[cappedAward += incrementalCappedAward\nawardPoolRemaining -= incrementalCappedAward]
N --> B
B --> O[sort by cappedAward desc\nthen duration desc\nthen address desc]
O --> P[build AwardedReferrerMetrics for each]
P --> Q[buildAggregatedMetrics\nwith awardPoolRemaining]
Q --> R[ReferrerLeaderboardRevShareCap]
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Refactors the rev-share-cap leaderboard/race algorithm in ens-referrals to operate on PriceEth/PriceUsdc objects end-to-end (instead of mixing bigint amounts and Price wrappers), and extends @ensnode/ensnode-sdk with additional price arithmetic helpers to support that.
Changes:
- Add
subtractPrices,minPrices, andmaxPriceshelpers to@ensnode/ensnode-sdkshared currencies utilities (with same-currency checks;subtractPricesdisallows negative results). - Extract
computeBaseRevenueContribution(rules, duration)helper inens-referralsand apply it across rev-share-cap metrics + leaderboard race processing. - Rename internal race state fields (
totalRevenueContributionAmount→totalRevenueContribution,cappedAwardAmount→cappedAward,wasQualified→hasQualified) and update race loop to usePriceobjects.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/ensnode-sdk/src/shared/currencies.ts | Adds subtractPrices, minPrices, maxPrices helpers for Price arithmetic/comparison. |
| packages/ensnode-sdk/src/shared/currencies.test.ts | Adds unit tests covering the new currency helpers (incl. mismatch-currency and negative-subtraction cases). |
| packages/ens-referrals/src/award-models/rev-share-cap/rules.ts | Adds computeBaseRevenueContribution and updates imports/constants usage. |
| packages/ens-referrals/src/award-models/rev-share-cap/metrics.ts | Switches base revenue contribution computation/validation to the shared helper. |
| packages/ens-referrals/src/award-models/rev-share-cap/leaderboard.ts | Rewrites race state + award pool logic to use PriceEth/PriceUsdc throughout. |
| .changeset/tidy-cobras-race.md | Changeset for @namehash/ens-referrals patch release describing internal refactor. |
| .changeset/quick-signals-align.md | Changeset for @ensnode/ensnode-sdk minor release adding new helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR refines the rev-share-cap leaderboard “race” calculation in ens-referrals to operate on typed PriceXXX objects end-to-end (instead of raw bigint amounts), and adds supporting currency helpers to @ensnode/ensnode-sdk. It also extracts the repeated base revenue contribution proration formula into a shared domain helper to keep the logic consistent across leaderboard + metrics.
Changes:
- Add
subtractPrices,minPrices, andmaxPriceshelpers to@ensnode/ensnode-sdkshared/currencies. - Refactor rev-share-cap leaderboard race state fields to be typed
PriceEth/PriceUsdcand update the loop to use price helpers for arithmetic. - Introduce and apply
computeBaseRevenueContribution(rules, duration)across rev-share-cap codepaths.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensnode-sdk/src/shared/currencies.ts | Adds new Price arithmetic/comparison helpers with same-currency enforcement and non-negative subtraction. |
| packages/ensnode-sdk/src/shared/currencies.test.ts | Adds unit tests covering new helpers (correctness + error cases). |
| packages/ens-referrals/src/award-models/rev-share-cap/rules.ts | Adds exported computeBaseRevenueContribution helper for prorated base contribution computation. |
| packages/ens-referrals/src/award-models/rev-share-cap/metrics.ts | Reuses computeBaseRevenueContribution to build/validate total base revenue contribution invariants. |
| packages/ens-referrals/src/award-models/rev-share-cap/leaderboard.ts | Refactors race state and award-pool accounting to use Price helpers consistently. |
| .changeset/tidy-cobras-race.md | Changeset for @namehash/ens-referrals patch release describing the refactor. |
| .changeset/quick-signals-align.md | Changeset for @ensnode/ensnode-sdk minor release adding the new helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@greptile |
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Goader Nice work 👍 Shared a few small suggestions please take the lead to merge when ready 👍
| * such value in argument order. | ||
| * @throws if not all prices have the same currency. | ||
| */ | ||
| export function maxPrices<const PriceType extends Price = Price>( |
There was a problem hiding this comment.
| export function maxPrices<const PriceType extends Price = Price>( | |
| export function maxPrice<const PriceType extends Price = Price>( |
| * such value in argument order. | ||
| * @throws if not all prices have the same currency. | ||
| */ | ||
| export function minPrices<const PriceType extends Price = Price>( |
There was a problem hiding this comment.
| export function minPrices<const PriceType extends Price = Price>( | |
| export function minPrice<const PriceType extends Price = Price>( |
| * @throws if not all prices have the same currency. | ||
| * @throws if the result would be negative ({@link CurrencyAmount} must be non-negative). | ||
| */ | ||
| export function subtractPrices<const PriceType extends Price = Price>( |
There was a problem hiding this comment.
What do you think about changing this to subtractPrice that always takes exactly two prices as input (price A and price B) where it then returns A - B?
Goal: Make it easier to think about what this operation does, even if it might not be quite as efficient when subtracting a lot of prices at once.
| /** | ||
| * Compute the base revenue contribution accrued over a given duration under | ||
| * rev-share-cap rules: `rules.baseAnnualRevenueContribution × (duration / 1 year)`. | ||
| * | ||
| * Uses exact integer bigint arithmetic (single floor division) so the result | ||
| * matches the aggregate for the same duration — avoids per-event truncation | ||
| * that would compound into a smaller sum than the aggregated value. | ||
| */ | ||
| export function computeBaseRevenueContribution( |
There was a problem hiding this comment.
| /** | |
| * Compute the base revenue contribution accrued over a given duration under | |
| * rev-share-cap rules: `rules.baseAnnualRevenueContribution × (duration / 1 year)`. | |
| * | |
| * Uses exact integer bigint arithmetic (single floor division) so the result | |
| * matches the aggregate for the same duration — avoids per-event truncation | |
| * that would compound into a smaller sum than the aggregated value. | |
| */ | |
| export function computeBaseRevenueContribution( | |
| /** | |
| * Calculate the base revenue contribution accrued over a given duration under | |
| * rev-share-cap rules: `rules.baseAnnualRevenueContribution × (duration / 1 year)`. | |
| * | |
| * Uses exact integer bigint arithmetic (single floor division) so the result | |
| * matches the aggregate for the same duration — avoids per-event truncation | |
| * that would compound into a smaller sum than the aggregated value. | |
| */ | |
| export function calcBaseRevenueContribution( |
Goal: There's a bunch of functions in the ENSAwards site where we use this "calc..." prefix. Seems nice to continue that idea here.
| * contribution met AND not admin-disqualified), and stays true thereafter. | ||
| */ | ||
| hasQualified: boolean; | ||
| /** Amount actually claimed from the award pool (the capped award). */ |
There was a problem hiding this comment.
| /** Amount actually claimed from the award pool (the capped award). */ | |
| /** Amount tentatively claimed from the award pool (capped by the available award pool). */ |
| "@ensnode/ensnode-sdk": minor | ||
| --- | ||
|
|
||
| Add `subtractPrices`, `minPrices`, and `maxPrices` helpers to `@ensnode/ensnode-sdk` (variadic, parallel to the existing `addPrices`; throw on mismatched currencies; `subtractPrices` additionally throws if the result would be negative). |
There was a problem hiding this comment.
Please see related feedback which may rename some of these
| let awardPoolRemaining: PriceUsdc = rules.awardPool; | ||
|
|
||
| for (const event of sortedEvents) { | ||
| const referrer = event.referrer; |
There was a problem hiding this comment.
To help make the code here more self-documenting, suggest the following:
- Rename
referrertoreferrerId. - Rename
statetoreferrerState
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/tidy-cobras-race.md:
- Line 5: The changeset text references a non-existent helper name
computeBaseRevenueContribution; update the changeset to reference the actual
exported helper calcBaseRevenueContribution (or alternatively rename the export
in rules.ts to computeBaseRevenueContribution) so the docs and release notes
match the code; ensure you also mention the renamed ReferrerRaceState fields
(totalRevenueContribution → totalRevenueContribution, cappedAward → cappedAward,
wasQualified → hasQualified) only if the changeset intended to describe those
exact symbol names, otherwise keep the helper name consistent with the exported
function calcBaseRevenueContribution.
In `@packages/ensnode-sdk/src/shared/currencies.ts`:
- Around line 183-249: DRY up repeated same-currency checks by adding a small
helper (e.g., assertSameCurrency or ensureSameCurrency) that accepts a variadic
list of Price (or PriceType) values and throws the existing "All prices must
have the same currency..." error when any differ; then replace the duplicate
checks in subtractPrice, minPrice, and maxPrice (and optionally addPrices) to
call this new helper (use firstPrice as the reference inside the helper and keep
current error text/behavior intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73f7d1ef-f9c3-4796-abb6-0a409a3b88bb
📒 Files selected for processing (7)
.changeset/quick-signals-align.md.changeset/tidy-cobras-race.mdpackages/ens-referrals/src/award-models/rev-share-cap/leaderboard.tspackages/ens-referrals/src/award-models/rev-share-cap/metrics.tspackages/ens-referrals/src/award-models/rev-share-cap/rules.tspackages/ensnode-sdk/src/shared/currencies.test.tspackages/ensnode-sdk/src/shared/currencies.ts
There was a problem hiding this comment.
Pull request overview
This PR refines the rev-share-cap leaderboard “race” calculation by converting internal mutable state to use typed PriceEth/PriceUsdc objects end-to-end, and introduces shared currency helpers in @ensnode/ensnode-sdk to make arithmetic/comparisons safer and more self-documenting.
Changes:
- Add
subtractPrice,minPrice, andmaxPricehelpers to@ensnode/ensnode-sdk(with tests). - Refactor the rev-share-cap leaderboard race state to store
Priceobjects instead of rawbigintamounts, and update the loop logic accordingly. - Extract the repeated prorated base revenue contribution formula into
calcBaseRevenueContribution(...)and reuse it across rev-share-cap modules.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensnode-sdk/src/shared/currencies.ts | Adds subtractPrice, minPrice, maxPrice helpers with same-currency validation and non-negative subtraction. |
| packages/ensnode-sdk/src/shared/currencies.test.ts | Adds unit tests covering success paths and error cases for the new helpers. |
| packages/ens-referrals/src/award-models/rev-share-cap/rules.ts | Introduces calcBaseRevenueContribution helper using single-division bigint arithmetic for prorating base annual contribution. |
| packages/ens-referrals/src/award-models/rev-share-cap/metrics.ts | Reuses calcBaseRevenueContribution and updates invariants/validation accordingly. |
| packages/ens-referrals/src/award-models/rev-share-cap/leaderboard.ts | Refactors the sequential race state/loop to use PriceEth/PriceUsdc and the new currency helpers. |
| .changeset/tidy-cobras-race.md | Changeset for @namehash/ens-referrals describing the internal leaderboard refactor + new helper adoption. |
| .changeset/quick-signals-align.md | Changeset for @ensnode/ensnode-sdk documenting the new currency helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refining the Leaderboard Calculation
closes: #1901
Summary
ensnode-sdkPriceXXXobjectsens-referralsWhy
ReferrerRaceState#1901Testing
Notes for Reviewer (Optional)
wasQualifiedtohasQualified(as opposed to the suggestion in RefineReferrerRaceState#1901), because it does check the disqualification status, and therefore represents the qualification as we understand it on the API levelPriceXXXsuggestions in RefineReferrerRaceState#1901 would make the implementation a little bit of a mess, since some objects would bePriceXXXobjects, while the others wouldn't be. I remade it to fully work withPriceXXXobjects to keep everything consistent and easy to read (had to introduce a few helpers)Pre-Review Checklist (Blocking)