fix pending queue #2954
fix pending queue #2954
Conversation
WalkthroughThe recent changes encompass a significant redesign of the transaction retrieval mechanisms in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant Store
Client->>Service: GetTXS(options...)
Service->>Store: GetTXS(options...)
Store-->>Service: Return Transactions
Service-->>Client: Return Transactions
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
trajan0x
force-pushed
the
fixes-pending-queue
branch
from
ec8306d to
405c5a6
Compare
Deploying sanguine-fe with
|
| Latest commit: |
b742103
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://abdac88c.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fixes-pending-queue.sanguine-fe.pages.dev |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
The pull request introduces flexible transaction querying by implementing the Option pattern, enhancing the handling of pending queues.
- Flexible Querying: Updated
ethergo/submitter/db/service.goto useOptionparameters forGetTXSandGetAllTXAttemptByStatus, allowing dynamic query options. - Mock Service Alignment: Modified
ethergo/submitter/db/mocks/service.goto align with the newOptionpattern. - Dynamic Max Results: Removed
MaxResultsPerChainconstant inethergo/submitter/db/txdb/store.go, replaced with runtime options. - Test Updates: Adjusted
ethergo/submitter/db_test.goandsubmitter_test.goto use the newWithStatusesoption. - Documentation Update: Updated comment in
ethergo/submitter/metrics.goto reflect the new constant nameDefaultMaxResultsPerChain.
7 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2954 +/- ##
===================================================
+ Coverage 25.31904% 25.33941% +0.02036%
===================================================
Files 790 790
Lines 56732 56789 +57
Branches 80 80
===================================================
+ Hits 14364 14390 +26
- Misses 40881 40912 +31
Partials 1487 1487
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
PR Summary
(updates since last review)
No major changes found since the last review.
7 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- ethergo/submitter/db/mocks/service.go (2 hunks)
- ethergo/submitter/db/service.go (2 hunks)
- ethergo/submitter/db/txdb/store.go (5 hunks)
- ethergo/submitter/db_test.go (5 hunks)
- ethergo/submitter/metrics.go (1 hunks)
- ethergo/submitter/queue.go (2 hunks)
- ethergo/submitter/submitter_test.go (3 hunks)
Additional comments not posted (27)
ethergo/submitter/db/service.go (8)
47-53: LGTM!The introduction of the
Optiontype andoptionsstruct enhances configurability and maintainability.
55-61: LGTM!The
OptionsFetcherinterface provides a clear contract for fetching options, improving code clarity.
63-69: LGTM!The methods
MaxResultsandStatusesprovide access to the encapsulated parameters, enhancing usability.
71-75: LGTM!The
DefaultMaxResultsPerChainconstant establishes a default limit for the maximum number of transactions, improving resiliency.
77-89: LGTM!The
ParseOptionsfunction enhances flexibility by allowing dynamic configuration of options.
91-96: LGTM!The
WithStatusesfunction improves code readability and maintainability by using a structured approach.
98-103: LGTM!The
WithMaxResultsfunction enhances configurability by allowing users to set result limits.
26-26: LGTM! But verify the function usage in the codebase.The changes to the method signatures improve flexibility and configurability.
However, ensure that all calls to
GetTXSandGetAllTXAttemptByStatusmatch the new signature.Also applies to: 34-34
Verification successful
Function usage verified successfully.
All calls to
GetTXSandGetAllTXAttemptByStatusin the codebase match the new signature usingOptioninstead ofStatus.
- Instances in
ethergo/submitter/db/mocks/service.go,ethergo/submitter/db/service.go,ethergo/submitter/submitter_test.go,ethergo/submitter/db_test.go,ethergo/submitter/db/txdb/store.go, andethergo/submitter/queue.goconfirm the correct usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetTXS` and `GetAllTXAttemptByStatus` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'GetTXS' rg --type go -A 5 $'GetAllTXAttemptByStatus'Length of output: 8841
ethergo/submitter/queue.go (2)
86-86: LGTM!The update to use
db.WithStatuses(...)improves readability and maintainability by grouping status parameters.
156-156: LGTM!The update to include
db.WithMaxResults(100)enhances performance by limiting the number of results returned.ethergo/submitter/db/mocks/service.go (2)
58-80: LGTM!The update to use
db.Optioninstead ofdb.Statusbroadens the flexibility of theGetAllTXAttemptByStatusmethod.
213-235: LGTM!The update to use
db.Optioninstead ofdb.Statusbroadens the flexibility of theGetTXSmethod.ethergo/submitter/db_test.go (5)
96-96: LGTM!The use of
db.WithStatuses(db.Pending)improves code readability and maintainability.
100-100: LGTM!Updating the expected maximum results per chain to
db.DefaultMaxResultsPerChainaligns with the new standards.
120-122: LGTM!The use of
db.WithStatuses(allStatuses...)enhances consistency in status handling.
240-240: LGTM!The use of
db.WithStatuses(allStatuses...)enhances consistency in status handling.
249-249: LGTM!The use of
db.WithStatuses(allStatuses...)enhances consistency in status handling.ethergo/submitter/metrics.go (1)
44-44: LGTM!Updating the comment to
DefaultMaxResultsPerChainclarifies the stopping condition for counting pending transactions.ethergo/submitter/db/txdb/store.go (6)
120-120: LGTM!The transition to an options-based approach enhances flexibility and configurability.
123-124: LGTM!The use of
db.ParseOptionsandmadeOptions.Statuses()improves code readability and maintainability.
145-145: LGTM!Using
madeOptions.MaxResults()for limiting results aligns with the new options-based approach.
178-178: LGTM!The transition to an options-based approach enhances flexibility and configurability.
181-182: LGTM!The use of
db.ParseOptionsandmadeOptions.Statuses()improves code readability and maintainability.
200-203: LGTM!Using
madeOptions.MaxResults()for limiting results aligns with the new options-based approach.ethergo/submitter/submitter_test.go (3)
302-302: LGTM!The update to use
db.WithStatuses(db.Stored)enhances clarity and maintainability.
359-359: LGTM!The update to use
db.WithStatuses(db.ReplacedOrConfirmed, db.Confirmed, db.Replaced)enhances clarity and maintainability.
401-401: LGTM!The update to use
db.WithStatuses(db.ReplacedOrConfirmed, db.Confirmed, db.Replaced)enhances clarity and maintainability.
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request focuses on increasing the chain pending queue count and includes several enhancements and fixes.
- Refactored
listener.go: Improved readability and maintainability by encapsulating thespan.SetAttributes(attribute.Bool("did_poll", didPoll))call within an anonymous deferred function. - Enhanced
chainPendingQueueinethergo/submitter/chain_queue.go: Improved transaction handling by adding new functions to manage transaction statuses and maximum result limits. - Updated
processQueueinethergo/submitter/queue.go: Enhanced configurability and resilience by removing hardcoded limits and allowing for more customizable queries. - Documentation and Tests: Updated documentation and test cases to reflect changes, improving code clarity and maintainability.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
Increased the maximum number of transaction attempts retrieved by the GetAllTXAttemptByStatus method to handle a larger volume of transactions in the pending queue.
- File Modified:
ethergo/submitter/queue.go - Change: Increased max transaction attempts from 100 to 1000 in
GetAllTXAttemptByStatus. - Impact: Potential for higher memory usage and longer processing times under heavy load.
- Consideration: Ensure system performance is not degraded with the increased load.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- ethergo/submitter/queue.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- ethergo/submitter/queue.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
Enhanced transaction status accuracy and control flow improvements in the pending queue.
-
File Modified:
ethergo/submitter/db/txdb/store.go- Change: Added condition to
MarkAllBeforeNonceReplacedOrConfirmedto exclude already confirmed transactions. - Impact: Prevents unnecessary updates to confirmed transactions.
- Change: Added condition to
-
File Modified:
ethergo/submitter/chain_queue.go- Change: Improved control flow in the listener's polling method.
- Impact: Better attribute handling and enhanced configurability.
-
File Modified:
ethergo/submitter/queue.go- Change: Updated test cases to utilize new options-based parameters.
- Impact: Enhanced clarity and maintainability of transaction queries.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
trajan0x
force-pushed
the
fixes-pending-queue
branch
from
59a52c3 to
b742103
Compare
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request focuses on enhancing the pending queue management by increasing the chain pending queue count and improving transaction status accuracy.
-
File Modified:
ethergo/submitter/db/txdb/store.go- Change: Added condition to
MarkAllBeforeNonceReplacedOrConfirmedto exclude already confirmed transactions. - Impact: Prevents unnecessary updates to confirmed transactions.
- Change: Added condition to
-
File Modified:
ethergo/submitter/chain_queue.go- Change: Improved control flow in the listener's polling method.
- Impact: Better attribute handling and enhanced configurability.
-
File Modified:
ethergo/submitter/queue.go- Change: Updated test cases to utilize new options-based parameters.
- Impact: Enhanced clarity and maintainability of transaction queries.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- ethergo/submitter/db/txdb/store.go (6 hunks)
Additional context used
GitHub Check: codecov/patch
ethergo/submitter/db/txdb/store.go
[warning] 121-121: ethergo/submitter/db/txdb/store.go#L121
Added line #L121 was not covered by tests
[warning] 124-125: ethergo/submitter/db/txdb/store.go#L124-L125
Added lines #L124 - L125 were not covered by tests
[warning] 146-146: ethergo/submitter/db/txdb/store.go#L146
Added line #L146 was not covered by tests
[warning] 179-179: ethergo/submitter/db/txdb/store.go#L179
Added line #L179 was not covered by tests
[warning] 182-184: ethergo/submitter/db/txdb/store.go#L182-L184
Added lines #L182 - L184 were not covered by tests
[warning] 201-201: ethergo/submitter/db/txdb/store.go#L201
Added line #L201 was not covered by tests
[warning] 204-204: ethergo/submitter/db/txdb/store.go#L204
Added line #L204 was not covered by tests
Additional comments not posted (2)
ethergo/submitter/db/txdb/store.go (2)
124-125: Ensure correctness of the helper function usage.The usage of
db.ParseOptionsandmadeOptions.Statusesshould be verified to ensure it correctly parses and handles the options.Verification successful
Helper function usage is correct.
The
ParseOptionsfunction correctly initializes theoptionsstruct, and theStatusesmethod on theoptionsstruct returns the expected statuses. The usage ofdb.ParseOptionsandmadeOptions.Statusesin the code snippet is appropriate.
ParseOptionsimplementation:ethergo/submitter/db/service.goOptiontype definition:ethergo/submitter/db/service.gooptionsstruct definition:ethergo/submitter/db/service.goOptionsFetcherinterface:ethergo/submitter/db/service.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `db.ParseOptions` function and its usage. # Test: Search for the function implementation. Expect: Ensure it handles options correctly. ast-grep --lang go --pattern $'func ParseOptions(options ...Option) Options { $$$ }'Length of output: 87
Script:
#!/bin/bash # Description: Locate the implementation of the `ParseOptions` function in the Go codebase. # Search for any function named `ParseOptions` in Go files. rg 'func ParseOptions' -t go -A 10Length of output: 624
Script:
#!/bin/bash # Description: Locate the definitions of `Option`, `options`, and `OptionsFetcher` in the Go codebase. # Search for the definition of `Option`. rg 'type Option' -t go -A 10 # Search for the definition of `options`. rg 'type options' -t go -A 10 # Search for the definition of `OptionsFetcher`. rg 'type OptionsFetcher' -t go -A 10Length of output: 8243
Tools
GitHub Check: codecov/patch
[warning] 124-125: ethergo/submitter/db/txdb/store.go#L124-L125
Added lines #L124 - L125 were not covered by tests
182-184: Ensure correctness of the helper function usage.The usage of
db.ParseOptionsandmadeOptions.Statusesshould be verified to ensure it correctly parses and handles the options.Tools
GitHub Check: codecov/patch
[warning] 182-184: ethergo/submitter/db/txdb/store.go#L182-L184
Added lines #L182 - L184 were not covered by tests
| @@ -123,10 +118,11 @@ | |||
| // GetTXS returns all transactions for a given address on a given (or any) chain id that match a given status. | |||
| // there is a limit of 50 transactions per chain id. The limit does not make any guarantees about the number of nonces per chain. | |||
| // the submitter will get only the most recent tx submitted for each chain so this can be used for gas pricing. | |||
| func (s *Store) GetTXS(ctx context.Context, fromAddress common.Address, chainID *big.Int, matchStatuses ...db.Status) (txs []db.TX, err error) { | |||
| func (s *Store) GetTXS(ctx context.Context, fromAddress common.Address, chainID *big.Int, options ...db.Option) (txs []db.TX, err error) { | |||
There was a problem hiding this comment.
Reminder: Add tests.
The new implementation of the GetTXS function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 121-121: ethergo/submitter/db/txdb/store.go#L121
Added line #L121 was not covered by tests
| @@ -147,7 +143,7 @@ | |||
| Where(fmt.Sprintf("%s IN ?", statusFieldName), inArgs). | |||
| Group(fmt.Sprintf("%s, %s", nonceFieldName, chainIDFieldName)). | |||
| Order(fmt.Sprintf("%s asc", nonceFieldName)). | |||
| Limit(MaxResultsPerChain) | |||
| Limit(madeOptions.MaxResults()) | |||
There was a problem hiding this comment.
Reminder: Add tests.
The limit on the number of results returned by the query should be covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 146-146: ethergo/submitter/db/txdb/store.go#L146
Added line #L146 was not covered by tests
| @@ -180,9 +176,12 @@ | |||
| } | |||
|
|
|||
| // GetAllTXAttemptByStatus returns all transactions for a given address on a given (or any) chain id that match a given status. | |||
| func (s *Store) GetAllTXAttemptByStatus(ctx context.Context, fromAddress common.Address, chainID *big.Int, matchStatuses ...db.Status) (txs []db.TX, err error) { | |||
| func (s *Store) GetAllTXAttemptByStatus(ctx context.Context, fromAddress common.Address, chainID *big.Int, options ...db.Option) (txs []db.TX, err error) { | |||
There was a problem hiding this comment.
Reminder: Add tests.
The new implementation of the GetAllTXAttemptByStatus function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 179-179: ethergo/submitter/db/txdb/store.go#L179
Added line #L179 was not covered by tests
| @@ -199,10 +198,10 @@ | |||
| subQuery := s.DB().Model(ÐTX{}). | |||
| Select(fmt.Sprintf("MAX(%s) as %s, %s, %s", idFieldName, idFieldName, nonceFieldName, chainIDFieldName)). | |||
| Where(query). | |||
| Where(fmt.Sprintf("%s IN ?", statusFieldName), statusToArgs(matchStatuses...)). | |||
| Where(fmt.Sprintf("%s IN ?", statusFieldName), inArgs). | |||
There was a problem hiding this comment.
Reminder: Add tests.
The limit on the number of results returned by the query should be covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 201-201: ethergo/submitter/db/txdb/store.go#L201
Added line #L201 was not covered by tests
| Group(fmt.Sprintf("%s, %s", nonceFieldName, chainIDFieldName)). | ||
| Order(fmt.Sprintf("%s asc", nonceFieldName)). | ||
| Limit(MaxResultsPerChain) | ||
| Limit(madeOptions.MaxResults()) |
There was a problem hiding this comment.
Reminder: Add tests.
The limit on the number of results returned by the query should be covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 204-204: ethergo/submitter/db/txdb/store.go#L204
Added line #L204 was not covered by tests
Description
prevent nonce overwrite:
https://github.com/synapsecns/sanguine/pull/2954/files#diff-0582778f84e9cb035d76b3d723832dd4fb9fff5ee4ed686ee42728172bf0a11aR56
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores