Repository navigation
fix: paginate bridge tracker activity endpoint 442 - #1888
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 756531d34e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sort.SliceStable(entries, func(i, j int) bool { | ||
| return entries[i].CreatedAt.After(entries[j].CreatedAt) | ||
| }) |
There was a problem hiding this comment.
Add a deterministic tie-breaker before slicing pages
When two or more bridges share a CreatedAt value—common for deposits in the same block—this comparator treats them as equal. The in-memory implementation reconstructs entries from map iteration (bridgetracker/activity.go:314-318), while the SQLite query also has no ORDER BY, so the relative order of tied entries can differ between requests; because SliceStable preserves that arbitrary input order, an entry can move across a page boundary and clients paging through results can receive duplicates or omit bridges. Sort equal timestamps by a unique stable field such as the global index before applying the offset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fda589d. Entries are now sorted by CreatedAt descending and, for equal timestamps, by global index descending (compared numerically with big.Int, since it can exceed 2^64), so tied entries keep the same relative order on every request regardless of the input order (map iteration in the in-memory cache, unordered SQL in the SQLite store). Added TestActivityCommandExecute_TiedCreatedAtOrderedByGlobalIndex and updated the API docs, which previously said the order of ties was not guaranteed.
fda589d to
73d9ef5
Compare
|
@claude review |
|
Claude finished @joanestebanr's task in 1m 0s —— View job PR review: paginate bridge tracker activity endpoint (#1888)I read the full diff against Verdict: the change is correct and well scoped, and the Codex tie-breaker finding is properly fixed. I found no 🔴 blockers, only a few 🟡 and 🟢 points. Findings🟡 Docs describe
🟢 Comparator allocates
🟢 Entries with no block timestamp sort as the newest.
🟢 Tie-break uniqueness depends on SQL keying. The SQLite table uses 🟢 Minor:
Behavior notes
🚀 What's New
🐛 Bug FixesNone. The tie-break ordering was a review fix within this PR, not a fix to existing behavior. 📋 Config UpdatesNone.
|
|
Thanks for the review. Addressed in edc99c2:
Not changed:
|
GET /activity/from/{from_address} returned every cached bridge for an
address in one unordered response. Add page_number/page_size query
params with the same names, defaults (1/20) and max (200) as the
bridge service's own paginated endpoints, sort results most recent
bridge first, and report the total match count in a new "count" field
(mirroring BridgesResult/ClaimsResult).
Fixes agglayer/pm#442
Pages are not a consistent snapshot: new bridges can cause duplicates across pages and be missed until the first page is refetched. Document how clients should page (count, last page) and handle this. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…tion The activity endpoint now paginates by default (first 20 bridges) and returns a new count field, so clients must page to get every bridge. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Entries sharing a CreatedAt (e.g. deposits in the same block) had an arbitrary relative order that could change between requests, letting a bridge move across a page boundary. Sort ties by global index descending, compared numerically. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Correct the creation_timestamp description (it is the bridge's deposit block timestamp, now the sort key), parse global indexes once before sorting, document how bridges without a block timestamp sort, rewrap a comment and test the max page_size. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
edc99c2 to
0a7e65d
Compare
🔄 Changes Summary
GET /tracker/v1/activity/from/{from_address}now acceptspage_number/page_sizequery params (same names, 1-based numbering, defaults and max as the bridge service's own paginated endpoints: default 1/20, max 200).CreatedAt, descending) before being paginated.ActivityResponsegains acountfield reporting the total number of bridges matchingfilterBridgesacross every page, mirroringBridgesResult/ClaimsResult's owncountfield.CurrentAPIRevisionbumped from 5 to 6 (moved tobridgetracker/api/revision.go), with the revision-history entry explaining the change.docs/bridgetracker/API.md, including the consistency caveats below.ActivityCachein-memory and the SQLite-backed store) already return the bounded, per-address bridge set, so sorting/pagination is applied once in the HTTP layer instead of duplicating it across both stores (and their generated mocks).GET /tracker/v1/activity/from/{from_address}: a client sending neitherpage_numbernorpage_sizeused to receive every bridge and now receives only the first 20 (most recent first), so it must page usingcount. The response schema itself is only extended (newcountfield). Henceapi_revision5 → 6.📋 Config Updates
🔌 API Updates
🔌 Bridge service API
🔌 Proxy API
🔌 Others API
GET /tracker/v1/activity/from/{from_address}— added optionalpage_number/page_sizequery params and acountfield in the response body. The schema change is additive, but the default page size (20) changes what a client sending no pagination params receives, so it can break clients that assumed the full list (see Breaking Changes).api_revisionbumped to 6.✅ Testing
bridgetracker/api/activity_command_test.gocovering most-recent-first sorting, default pagination, explicitpage_number/page_sizeslicing, a page past the end, and invalid pagination params (400).go build ./...,go test ./bridgetracker/...andgolangci-lint run ./bridgetracker/...all pass.dev_ui/SDK consumer of this endpoint exists in this repository.🐞 Issues
🔗 Related PRs
📝 Notes
domain.ActivityQuerier.GetActivity's signature would have required updating both store implementations, their generated mocks, and ~50 existing test call sites, for no real benefit given the per-address result set is already bounded (one wallet's bridge history, not the global bridges table).bridge.global_indexand refetch page 1 ifcountchanges. Documented indocs/bridgetracker/API.md. Cursor-based pagination was considered and left out to stay consistent with the bridge service.🤖 Generated with Claude Code