Conversation
Fix issue related to the AA wallet donations stuck in pending state
Add debug logs for the pending donations
deploy fix on the log decoding for AA wallets
Release Feb 10
This reverts commit a7a05b0.
Fix the failed donation issue
Fix allocated giv value
Fix allocated giv value
Release 4 16 2026
Hot fix for the boost data sync issue
hot fix for the boosting sync job
Hot fix for the sync boosting problem
Release hotfix: fix stale giveconomy sync event handling
Keep the public createUserByAddress IP cap in place while letting trusted internal requests authenticated with VERCEL_KEY skip the shared egress bucket. Add focused coverage for trusted bypass and public rate limiting. Made-with: Cursor
Make the live Optimism transaction tests skip when the RPC cannot return a historical receipt instead of failing unrelated PRs. Keep the assertions active when the external provider responds normally. Made-with: Cursor
Make the createUserByAddress limiter atomic so crashed requests cannot leave TTL-less counters behind, reuse the shared request trust/IP helpers across bootstrap and the resolver, and keep the rejection coverage aligned with the configured limit message. Made-with: Cursor
…limit-master hotfix trusted createUserByAddress rate limit
…s-duplicate-check add isRecipient to wallet address duplicate check
Contributor
WalkthroughThis PR refactors donation and Givback calculations to use monthly operational UTC windows instead of power rounds, implements rate limiting with trusted Vercel bypass for user creation, adds RPC trace fallback for transaction value extraction, and updates related repositories, tests, and utilities to support these changes. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant UserResolver
participant VercelCheck
participant Redis
Client->>UserResolver: createUserByAddress (with/without vercel_key header)
UserResolver->>VercelCheck: isTrustedVercelRequest(req)
alt Trusted Vercel Request
VercelCheck-->>UserResolver: true
UserResolver->>UserResolver: Return existing user<br/>(bypass rate limit)
else Non-Trusted Request
VercelCheck-->>UserResolver: false
UserResolver->>UserResolver: Extract client IP<br/>via getClientIP()
UserResolver->>Redis: Execute Lua script<br/>(INCR + EXPIRE)
alt Redis count <= limit (20)
Redis-->>UserResolver: count
UserResolver-->>Client: Create/return user
else Redis count > limit
Redis-->>UserResolver: count > 20
UserResolver-->>Client: Error: Rate limit exceeded
end
end
sequenceDiagram
participant TransactionService
participant BlockExplorer
participant DebugRPC
participant TraceRPC
TransactionService->>BlockExplorer: Fetch internal transactions<br/>(txlistinternal)
alt Explorer Success
BlockExplorer-->>TransactionService: Internal transactions
TransactionService-->>TransactionService: Return results
else Explorer Fails
BlockExplorer-->>TransactionService: Error
TransactionService->>DebugRPC: debug_traceTransaction<br/>(callTracer)
alt Debug Trace Success
DebugRPC-->>TransactionService: Trace data
TransactionService->>TransactionService: Extract value transfers<br/>recursively
TransactionService-->>TransactionService: Return transfers
else Debug Trace Fails
DebugRPC-->>TransactionService: Error
TransactionService->>TraceRPC: trace_transaction<br/>(fallback)
alt Trace Success
TraceRPC-->>TransactionService: Trace data
TransactionService->>TransactionService: Extract value transfers
TransactionService-->>TransactionService: Return transfers
else Trace Fails
TraceRPC-->>TransactionService: Error
TransactionService-->>TransactionService: Return empty array
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/repositories/projectAddressRepository.test.ts (1)
255-268: Add one focused test forfindRelatedAddressByWalletAddressrecipient-only filtering.These updates validate the
findAll...path well, but the behavior changed in production code is infindRelated.... Add a case asserting it returnsnullfor non-recipient rows and returns a row whenisRecipient = true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repositories/projectAddressRepository.test.ts` around lines 255 - 268, Add a focused test that calls findRelatedAddressByWalletAddress to verify recipient-only filtering: after creating the related address rows used in the current test (the ones queried by findAllRelatedAddressByWalletAddress), call findRelatedAddressByWalletAddress(newAddress1) and assert it returns null when isRecipient is false, then flip or create a row with isRecipient = true and call findRelatedAddressByWalletAddress(newAddress2) (or the updated address) and assert it returns the related address with project.id matching project.id; use the same identifiers (findRelatedAddressByWalletAddress, findAllRelatedAddressByWalletAddress, newAddress1, newAddress2, project) so the new assertions integrate alongside the existing assertions.src/utils/ipWhitelist.ts (1)
51-59: Consider using constant-time comparison for the secret key.The current string comparison
headerValue === vercelKeyis vulnerable to timing attacks, where an attacker could potentially infer the secret by measuring response times. While the practical risk is low in this context (rate limiting bypass), using a constant-time comparison is a security best practice for secret comparisons.🔒 Proposed fix using crypto.timingSafeEqual
+import { timingSafeEqual } from 'crypto'; + +const safeCompare = (a: string, b: string): boolean => { + const bufA = Buffer.from(a); + const bufB = Buffer.from(b); + if (bufA.length !== bufB.length) { + return false; + } + return timingSafeEqual(bufA, bufB); +}; + export const isTrustedVercelRequest = (req: any): boolean => { const vercelKey = process.env.VERCEL_KEY; const headerValue = getHeaderValue(req?.headers?.vercel_key); if (!vercelKey || !headerValue) { return false; } - return headerValue === vercelKey; + return safeCompare(headerValue, vercelKey); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/ipWhitelist.ts` around lines 51 - 59, The equality check in isTrustedVercelRequest uses a normal string comparison which can leak timing information; update isTrustedVercelRequest to perform a constant-time comparison by importing Node's crypto and using crypto.timingSafeEqual: convert headerValue and process.env.VERCEL_KEY to Buffer (e.g., Buffer.from(..., 'utf8')), ensure they are the same length (return false if lengths differ) and then call timingSafeEqual inside a try/catch, returning the boolean result and false on any error; reference the isTrustedVercelRequest function and the vercelKey/headerValue values when making the change.src/resolvers/userResolver.rateLimit.test.ts (1)
47-68: Consider adding a test for the boundary condition.The current tests cover trusted bypass and over-limit rejection. Consider adding a test that verifies the 20th request succeeds while the 21st fails, to ensure the boundary condition (
current > CREATE_USER_BY_ADDRESS_RATE_LIMIT) works correctly.🧪 Example boundary test
it('allows the 20th request but rejects the 21st', async () => { process.env.VERCEL_KEY = 'trusted-vercel-key'; const evalStub = sinon.stub(redis, 'eval'); const existingUser = { id: 1, walletAddress: '0x...' } as User; const getOne = sinon.stub().resolves(existingUser); const where = sinon.stub().returns({ getOne }); sinon.stub(User, 'createQueryBuilder').returns({ where } as never); sinon.stub(AppDataSource, 'getDataSource').returns({ getRepository: () => ({}), } as never); const resolver = new UserResolver({} as never); const ctx = { expressReq: { headers: {}, ip: '1.2.3.4' } } as never; // 20th request should succeed evalStub.resolves(20); const result = await resolver.createUserByAddress('0x...', ctx); assert.isTrue(result.existing); // 21st request should fail evalStub.resolves(21); await assertRejects( () => resolver.createUserByAddress('0x...', ctx), /Rate limit exceeded/, ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/resolvers/userResolver.rateLimit.test.ts` around lines 47 - 68, Add a boundary test for UserResolver.createUserByAddress that stubs redis.eval to return 20 then 21 and asserts the 20th request succeeds while the 21st is rejected; specifically, set up stubs for redis.eval, User.createQueryBuilder (or getRepository/getOne) to simulate an existing user, stub AppDataSource.getDataSource to return a repository, instantiate new UserResolver({} as never) and call createUserByAddress with a context object (expressReq.headers/ip), first resolving eval to 20 and asserting a successful/existing result, then resolving eval to 21 and asserting assertRejects with /Rate limit exceeded/.src/repositories/donationRepository.ts (1)
665-665: Explicitly convert result to number for type safety.PostgreSQL may return numeric aggregates as strings through certain drivers to preserve decimal precision. The function signature promises
Promise<number>, so explicit conversion prevents potential type mismatches.Suggested fix
- return result?.[0]?.totalUsdWithGivbackFactor ?? 0; + return parseFloat(result?.[0]?.totalUsdWithGivbackFactor) || 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repositories/donationRepository.ts` at line 665, The query result may return totalUsdWithGivbackFactor as a string; update the return in the function that currently returns result?.[0]?.totalUsdWithGivbackFactor ?? 0 to explicitly coerce the value to a number (e.g., using Number(...) or parseFloat(...)) before returning so the function's Promise<number> contract is satisfied; ensure you handle null/undefined by falling back to 0 if conversion yields NaN.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/repositories/donationRepository.test.ts`:
- Around line 2014-2026: The test currently asserts only that the aggregated sum
is at least the expected amount; capture a baseline by calling
getSumOfGivbackEligibleDonationsForSpecificRound() before inserting test
donations, then insert donation1/2/3, call the same function again and assert
that the exact delta equals valueUsd1*givbackFactor1 + valueUsd2*givbackFactor2
+ valueUsd3*givbackFactor3; for the exclusion cases follow the same
baseline/delta pattern: insert the excluded donation, call the sum function and
assert the delta is 0 for that excluded donation; keep using
Donation.delete(...) for cleanup and apply this baseline/delta fix to the other
affected tests that use getSumOfGivbackEligibleDonationsForSpecificRound and
Donation.delete.
In `@src/repositories/donationRepository.ts`:
- Around line 634-635: The SQL date filter in donationRepository.ts currently
uses strict inequalities (">" and "<") which excludes donations that fall
exactly on the window edges; update the WHERE clause to use inclusive
comparisons (">=" for the start boundary and "<" or preferably "<=" for the end
boundary depending on desired inclusivity) and, if following the suggested
approach, compute endDate as 16:00:00 on the first day of the next month so the
range is [startDate >= 16:00:00 on 1st, endDate < next month 16:00:00) or use
startDate >= and endDate <= if you want both endpoints included; modify the SQL
fragment and the code that builds startDate/endDate accordingly to ensure
donations exactly at 16:00:00 are captured.
In `@src/resolvers/donationResolver.test.ts`:
- Around line 2251-2259: Current test wrongly derives expectedGivbackFactor from
the saved donation itself (using donation.projectRank and
donation.bottomRankInRound), making the assertion self-fulfilling; instead, use
the known fixture values (the 80/20 snapshot: projectRank = 1 and
bottomRankInRound = 2) to compute the expected factor and assert against that.
Update the test to assert donation.projectRank === 1 and
donation.bottomRankInRound === 2 (or use the fixture variable names if present),
then call calculateGivbackFactorByRank with those literal/fixture values and
process.env.GIVBACK_MIN_FACTOR/MAX to compute expectedGivbackFactor and compare
it to donation.givbackFactor; keep the existing assertion for powerRound
(roundNumber) unchanged.
- Around line 6571-6586: The test currently uses assert.isAtLeast on
allocatedGivbacks.usdValueSentAmountInPowerRound which allows over-counting to
pass; change it to assert the exact delta introduced by donation1/2/3 by (1)
fetching the current allocatedGivbacks aggregate before inserting the three test
donations (store e.g. priorAllocated =
allocatedGivbacks.usdValueSentAmountInPowerRound), (2) insert donation1,
donation2, donation3 as done, (3) fetch allocatedGivbacks again into
result.data.data?.allocatedGivbacks and compute expectedSum as already defined,
then assert that newAllocated === priorAllocated + expectedSum (use strict
equality, not isAtLeast), and finally run the existing cleanup
Donation.delete([donation1.id, donation2.id, donation3.id]).
In `@src/services/chains/evm/transactionService.ts`:
- Around line 540-552: The empty-result branch in
getInternalTransactionsByTxHash currently returns [] when the explorer responds
with status: '0' or "no transactions found" without attempting the new RPC trace
fallback; update getInternalTransactionsByTxHash so that in the empty-result /
status:'0' path it calls getInternalTransactionsByTxHashFromRpcTrace (same as
the explorer-exception path), logs via txTrace (e.g.,
'getInternalTransactionsByTxHash:explorer_empty_fallback' and
'...:fallback_done' with internalTxsCount), and only return [] after that
fallback also yields no results; this ensures callers (and
getInternalTransfersFromTrace consumers) benefit from trace_transaction
retry-capable RPCs.
- Around line 367-389: collectValueTransfersFromCallTrace currently treats any
frame with a positive value as a transfer and can include reverted/errored
subcalls; update the function to first detect and skip errored/reverted frames
(e.g., check node.error, node.reverted or node.revertReason, node.failed, or
node.status === 0) with an early return before parsing value/adding a transfer,
and apply the identical guard to the other similar routine handling call traces
(the similar block around lines 437-453) so reverted internal calls are never
counted as value transfers.
In `@src/services/chains/index.test.ts`:
- Around line 13-27: The test helper
getTransactionInfoOrSkipWhenRpcIsUnavailable should not skip on
application-level TRANSACTION_NOT_FOUND from getTransactionInfoFromNetwork;
update the catch in getTransactionInfoOrSkipWhenRpcIsUnavailable to only call
context.skip() for transport-level RPC failures (e.g., network/connection
errors, request timeouts, or HTTP 5xx responses) by checking the thrown error's
class/properties (for example network error types, status/code fields or
messages like "ECONN", "ETIMEDOUT", or HTTP status >=500) and rethrow otherwise;
alternatively replace live chain calls with mocks for
getTransactionInfoFromNetwork to avoid flaky RPCs.
In `@src/services/givbackService.ts`:
- Around line 15-17: The current minFactor/maxFactor computation can produce NaN
if either minGivFactor or maxGivFactor is non-finite; before calling
Math.min/Math.max, sanitize inputs (Number.isFinite) and coerce non-finite
values to safe fallbacks (e.g., if one is finite use that value for both, or use
explicit defaults), then compute minFactor = Math.min(sanitizedMin,
sanitizedMax) and maxFactor = Math.max(sanitizedMin, sanitizedMax). Update the
helper that computes these bounds (the code that sets minFactor/maxFactor) so it
always returns finite numbers, and ensure calculateGivbackFactorByRank() uses
this sanitized result (and that calculateGivbackFactor() behavior remains
unchanged).
- Around line 73-74: Normalize the parsed rank coming from
getBottomGivbackRank() so both the factor and the returned value use the same
numeric value: parse the result once (e.g., const bottomRankNum =
Number(bottomRank)), validate Number.isFinite(bottomRankNum) && bottomRankNum >
0, use bottomRankNum when computing the factor and set bottomRankInRound to
bottomRankNum (falling back to 1 only if invalid). Update references in the
function that currently use Number(bottomRank) and the bottomRankInRound ternary
to use this normalized bottomRankNum.
---
Nitpick comments:
In `@src/repositories/donationRepository.ts`:
- Line 665: The query result may return totalUsdWithGivbackFactor as a string;
update the return in the function that currently returns
result?.[0]?.totalUsdWithGivbackFactor ?? 0 to explicitly coerce the value to a
number (e.g., using Number(...) or parseFloat(...)) before returning so the
function's Promise<number> contract is satisfied; ensure you handle
null/undefined by falling back to 0 if conversion yields NaN.
In `@src/repositories/projectAddressRepository.test.ts`:
- Around line 255-268: Add a focused test that calls
findRelatedAddressByWalletAddress to verify recipient-only filtering: after
creating the related address rows used in the current test (the ones queried by
findAllRelatedAddressByWalletAddress), call
findRelatedAddressByWalletAddress(newAddress1) and assert it returns null when
isRecipient is false, then flip or create a row with isRecipient = true and call
findRelatedAddressByWalletAddress(newAddress2) (or the updated address) and
assert it returns the related address with project.id matching project.id; use
the same identifiers (findRelatedAddressByWalletAddress,
findAllRelatedAddressByWalletAddress, newAddress1, newAddress2, project) so the
new assertions integrate alongside the existing assertions.
In `@src/resolvers/userResolver.rateLimit.test.ts`:
- Around line 47-68: Add a boundary test for UserResolver.createUserByAddress
that stubs redis.eval to return 20 then 21 and asserts the 20th request succeeds
while the 21st is rejected; specifically, set up stubs for redis.eval,
User.createQueryBuilder (or getRepository/getOne) to simulate an existing user,
stub AppDataSource.getDataSource to return a repository, instantiate new
UserResolver({} as never) and call createUserByAddress with a context object
(expressReq.headers/ip), first resolving eval to 20 and asserting a
successful/existing result, then resolving eval to 21 and asserting
assertRejects with /Rate limit exceeded/.
In `@src/utils/ipWhitelist.ts`:
- Around line 51-59: The equality check in isTrustedVercelRequest uses a normal
string comparison which can leak timing information; update
isTrustedVercelRequest to perform a constant-time comparison by importing Node's
crypto and using crypto.timingSafeEqual: convert headerValue and
process.env.VERCEL_KEY to Buffer (e.g., Buffer.from(..., 'utf8')), ensure they
are the same length (return false if lengths differ) and then call
timingSafeEqual inside a try/catch, returning the boolean result and false on
any error; reference the isTrustedVercelRequest function and the
vercelKey/headerValue values when making the change.
🪄 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: CHILL
Plan: Pro
Run ID: 34d9dfaf-f17a-42c3-ac1e-b1222543f13b
📒 Files selected for processing (14)
src/repositories/donationRepository.test.tssrc/repositories/donationRepository.tssrc/repositories/projectAddressRepository.test.tssrc/repositories/projectAddressRepository.tssrc/resolvers/donationResolver.test.tssrc/resolvers/userResolver.rateLimit.test.tssrc/resolvers/userResolver.tssrc/server/bootstrap.tssrc/server/cors.tssrc/services/chains/evm/transactionService.tssrc/services/chains/index.test.tssrc/services/givbackService.tssrc/utils/ipWhitelist.tssrc/utils/validators/projectValidator.test.ts
Comment on lines
2014
to
+2026
| const sumOfGivbackEligibleDonations = | ||
| await getSumOfGivbackEligibleDonationsForSpecificRound({ | ||
| powerRound, | ||
| }); | ||
| await getSumOfGivbackEligibleDonationsForSpecificRound({}); | ||
|
|
||
| assert.equal( | ||
| sumOfGivbackEligibleDonations, | ||
| const expectedSum = | ||
| valueUsd1 * givbackFactor1 + | ||
| valueUsd2 * givbackFactor2 + | ||
| valueUsd3 * givbackFactor3, | ||
| ); | ||
| valueUsd2 * givbackFactor2 + | ||
| valueUsd3 * givbackFactor3; | ||
|
|
||
| // Result should include at least these donations (may include others from DB) | ||
| assert.isAtLeast(sumOfGivbackEligibleDonations, expectedSum); | ||
|
|
||
| // Cleanup | ||
| await Donation.delete([donation1.id, donation2.id, donation3.id]); |
Contributor
There was a problem hiding this comment.
Strengthen these aggregation assertions.
These checks can still pass when the query is wrong: the happy-path test only proves the total is at least the expected value, and the exclusion tests only prove the total does not change after deleting the excluded row. If eligible donations are dropped, or unrelated rows are double-counted, this suite still goes green. Capture a baseline before inserting test data and assert the exact delta contributed by the eligible donations.
Suggested pattern
+ const baseline =
+ await getSumOfGivbackEligibleDonationsForSpecificRound({});
+
const sumOfGivbackEligibleDonations =
await getSumOfGivbackEligibleDonationsForSpecificRound({});
- assert.isAtLeast(sumOfGivbackEligibleDonations, expectedSum);
+ assert.equal(sumOfGivbackEligibleDonations - baseline, expectedSum);Apply the same baseline/delta pattern to the exclusion cases so they verify both:
- eligible donations are included, and
- the excluded donation contributes
0.
Also applies to: 2085-2097, 2158-2170, 2241-2253, 2292-2304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/repositories/donationRepository.test.ts` around lines 2014 - 2026, The
test currently asserts only that the aggregated sum is at least the expected
amount; capture a baseline by calling
getSumOfGivbackEligibleDonationsForSpecificRound() before inserting test
donations, then insert donation1/2/3, call the same function again and assert
that the exact delta equals valueUsd1*givbackFactor1 + valueUsd2*givbackFactor2
+ valueUsd3*givbackFactor3; for the exclusion cases follow the same
baseline/delta pattern: insert the excluded donation, call the sum function and
assert the delta is 0 for that excluded donation; keep using
Donation.delete(...) for cleanup and apply this baseline/delta fix to the other
affected tests that use getSumOfGivbackEligibleDonationsForSpecificRound and
Donation.delete.
Comment on lines
+634
to
+635
| AND "createdAt" > $1 | ||
| AND "createdAt" < $2 |
Contributor
There was a problem hiding this comment.
Boundary conditions may exclude donations at exact window edges.
The SQL uses strict inequality (> and <) which excludes donations at exactly 16:00:00.000 on the 1st and at 15:59:59.xxx on the last day. If the operational window should be inclusive of these boundaries (as the comments suggest "at 16:00 UTC"), consider using >= for the start boundary.
Suggested fix for inclusive boundaries
- AND "createdAt" > $1
- AND "createdAt" < $2
+ AND "createdAt" >= $1
+ AND "createdAt" < $2And adjust endDate to be 16:00:00 on the 1st of the next month for cleaner boundary:
- // End: Last day of current month at 15:59:59 UTC (or 1st of next month at 15:59:59)
- const endDate = new Date(Date.UTC(year, month + 1, 0, 15, 59, 59));
+ // End: 1st of next month at 16:00 UTC (exclusive)
+ const endDate = new Date(Date.UTC(year, month + 1, 1, 16, 0, 0));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| AND "createdAt" > $1 | |
| AND "createdAt" < $2 | |
| AND "createdAt" >= $1 | |
| AND "createdAt" < $2 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/repositories/donationRepository.ts` around lines 634 - 635, The SQL date
filter in donationRepository.ts currently uses strict inequalities (">" and "<")
which excludes donations that fall exactly on the window edges; update the WHERE
clause to use inclusive comparisons (">=" for the start boundary and "<" or
preferably "<=" for the end boundary depending on desired inclusivity) and, if
following the suggested approach, compute endDate as 16:00:00 on the first day
of the next month so the range is [startDate >= 16:00:00 on 1st, endDate < next
month 16:00:00) or use startDate >= and endDate <= if you want both endpoints
included; modify the SQL fragment and the code that builds startDate/endDate
accordingly to ensure donations exactly at 16:00:00 are captured.
Comment on lines
+2251
to
+2259
| const expectedGivbackFactor = calculateGivbackFactorByRank({ | ||
| projectRank: donation?.projectRank, | ||
| bottomRank: donation?.bottomRankInRound || 1, | ||
| minGivFactor: Number(process.env.GIVBACK_MIN_FACTOR), | ||
| maxGivFactor: Number(process.env.GIVBACK_MAX_FACTOR), | ||
| }); | ||
| assert.equal(donation?.givbackFactor, expectedGivbackFactor); | ||
| assert.equal(donation?.powerRound, roundNumber); | ||
| assert.equal(donation?.projectRank, 1); | ||
| assert.isAtLeast(donation?.projectRank || 0, 1); |
Contributor
There was a problem hiding this comment.
Don't derive the expected rank/factor from the saved donation itself.
This assertion is now self-fulfilling: expectedGivbackFactor is computed from donation.projectRank and donation.bottomRankInRound, which were produced by the same calculateGivbackFactor() call you're trying to validate. If the service stores the wrong rank pair, this still passes. Use the known fixture instead — with the 80/20 snapshot setup, this project should be rank 1 among 2 ranked projects.
Suggested fix
- const expectedGivbackFactor = calculateGivbackFactorByRank({
- projectRank: donation?.projectRank,
- bottomRank: donation?.bottomRankInRound || 1,
+ assert.equal(donation?.projectRank, 1);
+ assert.equal(donation?.bottomRankInRound, 2);
+ const expectedGivbackFactor = calculateGivbackFactorByRank({
+ projectRank: 1,
+ bottomRank: 2,
minGivFactor: Number(process.env.GIVBACK_MIN_FACTOR),
maxGivFactor: Number(process.env.GIVBACK_MAX_FACTOR),
});
assert.equal(donation?.givbackFactor, expectedGivbackFactor);
assert.equal(donation?.powerRound, roundNumber);
- assert.isAtLeast(donation?.projectRank || 0, 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const expectedGivbackFactor = calculateGivbackFactorByRank({ | |
| projectRank: donation?.projectRank, | |
| bottomRank: donation?.bottomRankInRound || 1, | |
| minGivFactor: Number(process.env.GIVBACK_MIN_FACTOR), | |
| maxGivFactor: Number(process.env.GIVBACK_MAX_FACTOR), | |
| }); | |
| assert.equal(donation?.givbackFactor, expectedGivbackFactor); | |
| assert.equal(donation?.powerRound, roundNumber); | |
| assert.equal(donation?.projectRank, 1); | |
| assert.isAtLeast(donation?.projectRank || 0, 1); | |
| assert.equal(donation?.projectRank, 1); | |
| assert.equal(donation?.bottomRankInRound, 2); | |
| const expectedGivbackFactor = calculateGivbackFactorByRank({ | |
| projectRank: 1, | |
| bottomRank: 2, | |
| minGivFactor: Number(process.env.GIVBACK_MIN_FACTOR), | |
| maxGivFactor: Number(process.env.GIVBACK_MAX_FACTOR), | |
| }); | |
| assert.equal(donation?.givbackFactor, expectedGivbackFactor); | |
| assert.equal(donation?.powerRound, roundNumber); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/resolvers/donationResolver.test.ts` around lines 2251 - 2259, Current
test wrongly derives expectedGivbackFactor from the saved donation itself (using
donation.projectRank and donation.bottomRankInRound), making the assertion
self-fulfilling; instead, use the known fixture values (the 80/20 snapshot:
projectRank = 1 and bottomRankInRound = 2) to compute the expected factor and
assert against that. Update the test to assert donation.projectRank === 1 and
donation.bottomRankInRound === 2 (or use the fixture variable names if present),
then call calculateGivbackFactorByRank with those literal/fixture values and
process.env.GIVBACK_MIN_FACTOR/MAX to compute expectedGivbackFactor and compare
it to donation.givbackFactor; keep the existing assertion for powerRound
(roundNumber) unchanged.
Comment on lines
6571
to
+6586
| const allocatedGivbacks = result.data.data?.allocatedGivbacks; | ||
| assert.isNotNull(allocatedGivbacks); | ||
| assert.equal( | ||
|
|
||
| const expectedSum = | ||
| valueUsd1 * givbackFactor1 + | ||
| valueUsd2 * givbackFactor2 + | ||
| valueUsd3 * givbackFactor3; | ||
|
|
||
| // Result should include at least these donations (may include others from DB) | ||
| assert.isAtLeast( | ||
| allocatedGivbacks.usdValueSentAmountInPowerRound, | ||
| valueUsd1 * givbackFactor1 + valueUsd2 * givbackFactor2, | ||
| expectedSum, | ||
| ); | ||
|
|
||
| // Cleanup | ||
| await Donation.delete([donation1.id, donation2.id, donation3.id]); |
Contributor
There was a problem hiding this comment.
Make this integration assertion exact, not lower-bounded.
assert.isAtLeast(...) lets resolver over-counting slip through whenever the current window already contains other qualifying donations. This test should verify the exact delta introduced by the three rows it creates, otherwise a broken aggregation can still pass.
Suggested pattern
+ const baselineResult = await axios.post(
+ graphqlUrl,
+ {
+ query: allocatedGivbacksQuery,
+ variables: { refreshCache: true },
+ },
+ {},
+ );
+ const baseline =
+ baselineResult.data.data?.allocatedGivbacks.usdValueSentAmountInPowerRound;
const result = await axios.post(
graphqlUrl,
{
query: allocatedGivbacksQuery,
@@
- assert.isAtLeast(
- allocatedGivbacks.usdValueSentAmountInPowerRound,
- expectedSum,
- );
+ assert.equal(
+ allocatedGivbacks.usdValueSentAmountInPowerRound - baseline,
+ expectedSum,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/resolvers/donationResolver.test.ts` around lines 6571 - 6586, The test
currently uses assert.isAtLeast on
allocatedGivbacks.usdValueSentAmountInPowerRound which allows over-counting to
pass; change it to assert the exact delta introduced by donation1/2/3 by (1)
fetching the current allocatedGivbacks aggregate before inserting the three test
donations (store e.g. priorAllocated =
allocatedGivbacks.usdValueSentAmountInPowerRound), (2) insert donation1,
donation2, donation3 as done, (3) fetch allocatedGivbacks again into
result.data.data?.allocatedGivbacks and compute expectedSum as already defined,
then assert that newAllocated === priorAllocated + expectedSum (use strict
equality, not isAtLeast), and finally run the existing cleanup
Donation.delete([donation1.id, donation2.id, donation3.id]).
Comment on lines
+367
to
+389
| function collectValueTransfersFromCallTrace( | ||
| node: any, | ||
| transfers: Array<{ from: string; to: string; value: string }>, | ||
| ) { | ||
| if (!node || typeof node !== 'object') return; | ||
|
|
||
| const from = typeof node.from === 'string' ? node.from.toLowerCase() : ''; | ||
| const to = typeof node.to === 'string' ? node.to.toLowerCase() : ''; | ||
| const value = parseTraceValueToWei(node.value); | ||
|
|
||
| if (from && to && value.gt(0)) { | ||
| transfers.push({ | ||
| from, | ||
| to, | ||
| value: value.toString(), | ||
| }); | ||
| } | ||
|
|
||
| if (Array.isArray(node.calls)) { | ||
| for (const call of node.calls) { | ||
| collectValueTransfersFromCallTrace(call, transfers); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Skip reverted trace frames when deriving internal transfers.
Both trace formats can include errored subcalls. Right now any frame with a positive value is treated as a real transfer, so a reverted internal call can still be picked as the AA donation transfer and return the wrong from/to/amount.
💡 Minimal hardening
function collectValueTransfersFromCallTrace(
node: any,
transfers: Array<{ from: string; to: string; value: string }>,
) {
if (!node || typeof node !== 'object') return;
+ if (node.error) return;
const from = typeof node.from === 'string' ? node.from.toLowerCase() : '';
const to = typeof node.to === 'string' ? node.to.toLowerCase() : '';
const value = parseTraceValueToWei(node.value);
@@
for (const call of calls) {
+ if (call?.error) continue;
const from =
typeof call?.action?.from === 'string'
? call.action.from.toLowerCase()
: '';Also applies to: 437-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/chains/evm/transactionService.ts` around lines 367 - 389,
collectValueTransfersFromCallTrace currently treats any frame with a positive
value as a transfer and can include reverted/errored subcalls; update the
function to first detect and skip errored/reverted frames (e.g., check
node.error, node.reverted or node.revertReason, node.failed, or node.status ===
0) with an early return before parsing value/adding a transfer, and apply the
identical guard to the other similar routine handling call traces (the similar
block around lines 437-453) so reverted internal calls are never counted as
value transfers.
Comment on lines
+540
to
+552
| txTrace( | ||
| params, | ||
| 'getInternalTransactionsByTxHash:explorer_failed_fallback', | ||
| { | ||
| error: e?.message, | ||
| }, | ||
| ); | ||
| const internalTxsFromTrace = | ||
| await getInternalTransactionsByTxHashFromRpcTrace(params); | ||
| txTrace(params, 'getInternalTransactionsByTxHash:fallback_done', { | ||
| internalTxsCount: internalTxsFromTrace.length, | ||
| }); | ||
| return []; | ||
| return internalTxsFromTrace; |
Contributor
There was a problem hiding this comment.
Wire the new RPC trace fallback into the empty-result path too.
This only runs when the explorer request throws. When the explorer returns status: '0' with an empty/"no transactions found" payload, getInternalTransactionsByTxHash() returns [] earlier and the AA callers fall back to the older getInternalTransfersFromTrace() helper, which still lacks the trace_transaction retry. Providers that expose trace_transaction but not debug_traceTransaction will still produce false negatives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/chains/evm/transactionService.ts` around lines 540 - 552, The
empty-result branch in getInternalTransactionsByTxHash currently returns [] when
the explorer responds with status: '0' or "no transactions found" without
attempting the new RPC trace fallback; update getInternalTransactionsByTxHash so
that in the empty-result / status:'0' path it calls
getInternalTransactionsByTxHashFromRpcTrace (same as the explorer-exception
path), logs via txTrace (e.g.,
'getInternalTransactionsByTxHash:explorer_empty_fallback' and
'...:fallback_done' with internalTxsCount), and only return [] after that
fallback also yields no results; this ensures callers (and
getInternalTransfersFromTrace consumers) benefit from trace_transaction
retry-capable RPCs.
Comment on lines
+13
to
+27
| async function getTransactionInfoOrSkipWhenRpcIsUnavailable( | ||
| context: Mocha.Context, | ||
| input: Parameters<typeof getTransactionInfoFromNetwork>[0], | ||
| ) { | ||
| try { | ||
| return await getTransactionInfoFromNetwork(input); | ||
| } catch (error) { | ||
| if ((error as Error).message === errorMessages.TRANSACTION_NOT_FOUND) { | ||
| // These assertions hit live Optimism RPCs and can intermittently lose | ||
| // historical receipts even though the transactions are valid. | ||
| context.skip(); | ||
| } | ||
| throw error; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don't skip on TRANSACTION_NOT_FOUND from the code under test.
TRANSACTION_NOT_FOUND is also the normal product error when lookup/resolution breaks, so these tests now go green for real regressions as well as flaky RPCs. Please only skip on transport-level RPC failures (timeout/5xx/connection errors), or mock the chain calls instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/chains/index.test.ts` around lines 13 - 27, The test helper
getTransactionInfoOrSkipWhenRpcIsUnavailable should not skip on
application-level TRANSACTION_NOT_FOUND from getTransactionInfoFromNetwork;
update the catch in getTransactionInfoOrSkipWhenRpcIsUnavailable to only call
context.skip() for transport-level RPC failures (e.g., network/connection
errors, request timeouts, or HTTP 5xx responses) by checking the thrown error's
class/properties (for example network error types, status/code fields or
messages like "ECONN", "ETIMEDOUT", or HTTP status >=500) and rethrow otherwise;
alternatively replace live chain calls with mocks for
getTransactionInfoFromNetwork to avoid flaky RPCs.
Comment on lines
+15
to
+17
| // Keep configured bounds stable even if env values are swapped. | ||
| const minFactor = Math.min(minGivFactor, maxGivFactor); | ||
| const maxFactor = Math.max(minGivFactor, maxGivFactor); |
Contributor
There was a problem hiding this comment.
Sanitize non-finite factor bounds before clamping.
Math.min()/Math.max() return NaN when either input is NaN, so this exported helper can still return NaN if a caller passes env-derived values directly. calculateGivbackFactor() protects its own call site, but calculateGivbackFactorByRank() is now public and should be safe on its own.
Suggested fix
- const minFactor = Math.min(minGivFactor, maxGivFactor);
- const maxFactor = Math.max(minGivFactor, maxGivFactor);
+ const safeMinGivFactor = Number.isFinite(minGivFactor) ? minGivFactor : 0;
+ const safeMaxGivFactor = Number.isFinite(maxGivFactor)
+ ? maxGivFactor
+ : safeMinGivFactor;
+
+ const minFactor = Math.min(safeMinGivFactor, safeMaxGivFactor);
+ const maxFactor = Math.max(safeMinGivFactor, safeMaxGivFactor);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Keep configured bounds stable even if env values are swapped. | |
| const minFactor = Math.min(minGivFactor, maxGivFactor); | |
| const maxFactor = Math.max(minGivFactor, maxGivFactor); | |
| // Keep configured bounds stable even if env values are swapped. | |
| const safeMinGivFactor = Number.isFinite(minGivFactor) ? minGivFactor : 0; | |
| const safeMaxGivFactor = Number.isFinite(maxGivFactor) | |
| ? maxGivFactor | |
| : safeMinGivFactor; | |
| const minFactor = Math.min(safeMinGivFactor, safeMaxGivFactor); | |
| const maxFactor = Math.max(safeMinGivFactor, safeMaxGivFactor); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/givbackService.ts` around lines 15 - 17, The current
minFactor/maxFactor computation can produce NaN if either minGivFactor or
maxGivFactor is non-finite; before calling Math.min/Math.max, sanitize inputs
(Number.isFinite) and coerce non-finite values to safe fallbacks (e.g., if one
is finite use that value for both, or use explicit defaults), then compute
minFactor = Math.min(sanitizedMin, sanitizedMax) and maxFactor =
Math.max(sanitizedMin, sanitizedMax). Update the helper that computes these
bounds (the code that sets minFactor/maxFactor) so it always returns finite
numbers, and ensure calculateGivbackFactorByRank() uses this sanitized result
(and that calculateGivbackFactor() behavior remains unchanged).
Comment on lines
+73
to
+74
| bottomRankInRound: | ||
| Number.isFinite(bottomRank) && bottomRank > 0 ? bottomRank : 1, |
Contributor
There was a problem hiding this comment.
Normalize bottomRankInRound from the parsed rank.
The factor calculation uses Number(bottomRank), but the returned bottomRankInRound still checks the raw value. If getBottomGivbackRank() ever comes back as a numeric string, the factor is computed with that parsed rank while the stored bottomRankInRound collapses to 1. That leaves donation metadata inconsistent with the applied factor.
Suggested fix
+ const normalizedBottomRank = Number(bottomRank);
+
return {
givbackFactor,
projectRank: projectGivbackRankView?.powerRank,
bottomRankInRound:
- Number.isFinite(bottomRank) && bottomRank > 0 ? bottomRank : 1,
+ Number.isFinite(normalizedBottomRank) && normalizedBottomRank > 0
+ ? normalizedBottomRank
+ : 1,
powerRound: powerRound?.round as number,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bottomRankInRound: | |
| Number.isFinite(bottomRank) && bottomRank > 0 ? bottomRank : 1, | |
| const normalizedBottomRank = Number(bottomRank); | |
| return { | |
| givbackFactor, | |
| projectRank: projectGivbackRankView?.powerRank, | |
| bottomRankInRound: | |
| Number.isFinite(normalizedBottomRank) && normalizedBottomRank > 0 | |
| ? normalizedBottomRank | |
| : 1, | |
| powerRound: powerRound?.round as number, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/givbackService.ts` around lines 73 - 74, Normalize the parsed
rank coming from getBottomGivbackRank() so both the factor and the returned
value use the same numeric value: parse the result once (e.g., const
bottomRankNum = Number(bottomRank)), validate Number.isFinite(bottomRankNum) &&
bottomRankNum > 0, use bottomRankNum when computing the factor and set
bottomRankInRound to bottomRankNum (falling back to 1 only if invalid). Update
references in the function that currently use Number(bottomRank) and the
bottomRankInRound ternary to use this normalized bottomRankNum.
ae2079
approved these changes
Apr 28, 2026
kkatusic
approved these changes
Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements