fix bridge history GetL2UnclaimedWithdrawalsByAddress #1760
Conversation
WalkthroughThis PR combines a version bump (v4.7.1 to v4.7.2) with two targeted improvements to the bridge history API: adding early error handling when no withdrawal records are found, and increasing the query result limit from 500 to 10000 records. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1760 +/- ##
===========================================
- Coverage 36.54% 36.53% -0.01%
===========================================
Files 247 247
Lines 21186 21190 +4
===========================================
Hits 7742 7742
- Misses 12614 12618 +4
Partials 830 830
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bridge-history-api/internal/logic/history_logic.go (1)
94-103: Empty result is now treated as an error, which changes API semantics and breaks empty‑set cachingThe new guard:
if len(txHistoryInfos) == 0 { log.Error("failed to get L2 claimable withdrawals by address len = 0", "address", address) return nil, 0, errors.New("unexpected error") }introduces two regressions:
Semantics: Previously, “no unclaimed withdrawals” was a valid state: the method returned an empty slice and
total=0. Now the same situation surfaces as an error, which will likely be exposed as a 5xx to clients, unlikeGetL2WithdrawalsByAddress/GetTxsByAddresswhich still treat empty sets as success.Caching: With the early return,
processAndCacheTxHistoryInfois never called when the result is empty, so the"empty_page"placeholder is not written. For addresses with no unclaimed withdrawals, each call will miss the cache and re-hit the DB, defeating the existing empty-set caching optimization.Unless there is a strong invariant that this function must always return at least one record (and callers are coded accordingly), this looks like a behavioral bug rather than a fix.
Consider reverting this guard, or at most keeping a log line but still flowing through to
processAndCacheTxHistoryInfoand returning an empty slice on success:- if len(txHistoryInfos) == 0 { - log.Error("failed to get L2 claimable withdrawals by address len = 0", "address", address) - return nil, 0, errors.New("unexpected error") - } + if len(txHistoryInfos) == 0 { + log.Info("no L2 claimable withdrawals for address", "address", address) + }This preserves existing API behavior and cache behavior while still giving visibility into unexpectedly empty cases via logs.
🧹 Nitpick comments (1)
bridge-history-api/internal/orm/cross_message.go (1)
151-165: Consider making the 10000 limit configurable and aligned with other query limitsBumping the limit from 500 to 10000 makes sense for correctness on very active addresses, but it also increases worst‑case DB and Redis load for this path. It’s also now inconsistent with the 500 limits in
GetL2WithdrawalsByAddressandGetTxsByAddress.Consider:
- Extracting this limit into a named constant (or config) rather than an inline
10000.- Reassessing whether 10000 is the right order of magnitude given expected per‑address volume and cache TTL.
This keeps the behavior change while making it easier to tune if load characteristics change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bridge-history-api/internal/logic/history_logic.go(1 hunks)bridge-history-api/internal/orm/cross_message.go(1 hunks)common/version/version.go(1 hunks)
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: test
🔇 Additional comments (1)
common/version/version.go (1)
8-31: Version bump looks good; ensure external references are kept in syncThe bump of
tagtov4.7.2cleanly updates the computedVersionstring without changing any logic. Just make sure build/release metadata (git tags, changelogs, deployment configs) are aligned with this new version.
georgehao
deleted the
fix/bridge_history_GetL2UnclaimedWithdrawalsByAddress
branch
Purpose or design rationale of this PR
Fix issue where
bridge-history-apifails to return results when a user has >500 pending withdrawals (see discussion).PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Bug Fixes
Improvements
Version
✏️ Tip: You can customize this high-level summary in your review settings.