-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: queue block process updates waiting on pre-computation to be finished before starting next #370
Conversation
…nished before starting next
Warning Rate limit exceeded@p0mvn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 23 seconds before requesting another review. 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. WalkthroughIn version Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IngestUseCase
participant PricingWorker
Client->>IngestUseCase: Start Processing Blocks
IngestUseCase->>IngestUseCase: Check hasProcessedFirstBlock
alt is false
IngestUseCase->>IngestUseCase: Process first block
IngestUseCase->>IngestUseCase: Set hasProcessedFirstBlock true
IngestUseCase->>PricingWorker: Invoke UpdatePricesSync
else is true
IngestUseCase->>IngestUseCase: Wait for firstBlockWg
IngestUseCase->>PricingWorker: Invoke UpdatePricesAsync
end
PricingWorker->>Client: Block Processing Complete
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range and nitpick comments (13)
CHANGELOG.md (4)
Line range hint
87-87
: Fix grammatical issue: "fallback" should be "fall back".The word “fallback” is a noun. The verb is spelled with a white space.
- Hack to fallback to precision of 18 when estimating spot price for Astroport pools via quotes. + Hack to fall back to precision of 18 when estimating spot price for Astroport pools via quotes.
Line range hint
139-139
: Fix grammatical issue: "ingest" should be "ingests" or "ingested".There seems to be a noun/verb agreement error.
- Pricing ingest worker + Pricing ingests worker
Line range hint
203-203
: Fix typographical issue: "clean ups" should be "clean-ups".This expression is normally spelled as one or with a hyphen.
- light clean ups from the data ingest refactor. + light clean-ups from the data ingest refactor.
Line range hint
266-266
: Fix typographical issue: "Human readable" should be "Human-readable".This word is normally spelled with a hyphen.
- Human readable denom params in router + Human-readable denom params in routerdocs/architecture/ingest.md (9)
Line range hint
96-96
: Fix grammatical issue: "in" should be "at".The preposition “at” seems more likely in this position than the preposition “in”.
- in a previous height. + at a previous height.
Line range hint
122-122
: Fix grammatical issue: Misplaced comma.The comma may be misplaced. Try moving it before ‘and’, or removing it altogether.
- TotalLiquidity and, finally add the new contributions to the total. + TotalLiquidity, and finally add the new contributions to the total.
Line range hint
122-122
: Fix grammatical issue: Missing comma.Possible missing comma found.
- TotalLiquidity and, finally add the new contributions to the total. + TotalLiquidity, and finally, add the new contributions to the total.
Line range hint
130-130
: Fix style issue: Use "can" instead of "able to".As a shorter alternative for ‘able to’, consider using “can”.
- we are able to recompute the liquidity capitalization + we can recompute the liquidity capitalization
Line range hint
84-84
: Fix Markdownlint issue: Use dashes for unordered lists.Expected: dash; Actual: asterisk
- * Value of all pool liquidity of a token across all pools - * Value of all pool balances in USDC - * This happens at cold start or if any error is returned in a previois block. - * This assumes that all other pools have already been ingested in a previous height. + - Value of all pool liquidity of a token across all pools + - Value of all pool balances in USDC + - This happens at cold start or if any error is returned in a previois block. + - This assumes that all other pools have already been ingested in a previous height.Also applies to: 86-86, 94-94, 96-96
Line range hint
84-84
: Fix Markdownlint issue: Unordered list indentation.Expected: 0; Actual: 2
- * Value of all pool liquidity of a token across all pools - * Value of all pool balances in USDC + - Value of all pool liquidity of a token across all pools + - Value of all pool balances in USDCAlso applies to: 86-86
Line range hint
159-159
: Fix Markdownlint issue: Trailing spaces.Expected: 0 or 2; Actual: 1
- +
Line range hint
41-47
: Fix Markdownlint issue: Hard tabs.Column: 1
- if x < y: - return x - while x >= y: - x -= y - return x + if x < y: + return x + while x >= y: + x -= y + return xAlso applies to: 108-112, 159-164
Line range hint
12-12
: Fix Markdownlint issue: Lists should be surrounded by blank lines.- 1. Token liquidity capitalization across all pools - * Value of all pool liquidity of a token across all pools - 2. Pool liqudity capitalization - * Value of all pool balances in USDC + + 1. Token liquidity capitalization across all pools + - Value of all pool liquidity of a token across all pools + 2. Pool liqudity capitalization + - Value of all pool balances in USDC +Also applies to: 83-83, 84-84, 85-85, 86-86, 89-89, 93-93, 146-146, 164-164
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- docs/architecture/ingest.md (1 hunks)
- domain/pricing.go (1 hunks)
- ingest/usecase/ingest_usecase.go (4 hunks)
- tokens/usecase/pricing/worker/pricing_worker.go (1 hunks)
Additional context used
LanguageTool
CHANGELOG.md
[grammar] ~87-~87: The word “fallback” is a noun. The verb is spelled with a white space.
Context: ...ther & clean up tokens/prices - Hack to fallback to precision of 18 when estimating spot...(NOUN_VERB_CONFUSION)
[grammar] ~139-~139: There seems to be a noun/verb agreement error. Did you mean “ingests” or “ingested”?
Context: ...utes parameter ## v0.17.0 - Pricing ingest worker - Remove support for unlisted ...(SINGULAR_NOUN_VERB_AGREEMENT)
[misspelling] ~203-~203: This expression is normally spelled as one or with a hyphen.
Context: ...ub.com//pull/143) light clean ups from the data ingest refactor. - [#14...(EN_COMPOUNDS_CLEAN_UPS)
[misspelling] ~266-~266: This word is normally spelled with a hyphen.
Context: ...85) /tokens/prices endpoint ## v0.5.0 Human readable denom params in router - [#84](https...(EN_COMPOUNDS_HUMAN_READABLE)
docs/architecture/ingest.md
[uncategorized] ~96-~96: The preposition “at” seems more likely in this position than the preposition “in”.
Context: ... other pools have already been ingested in a previous height. In the current sys...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_AT)
[typographical] ~122-~122: The comma may be misplaced. Try moving it before ‘and’, or removing it altogether.
Context: ...ibution of that pool toTotalLiquidity
and, finally add the new contributions to th...(COMMA_AFTER_AND)
[uncategorized] ~122-~122: Possible missing comma found.
Context: ...n of that pool toTotalLiquidity
and, finally add the new contributions to the total ...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~130-~130: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...nd prices of each token in the pool, we are able to recompute the liquidity capitalization ...(BE_ABLE_TO)
Markdownlint
CHANGELOG.md
57-57: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
191-191: null
Bare URL used(MD034, no-bare-urls)
docs/architecture/ingest.md
84-84: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
86-86: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
94-94: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
96-96: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
84-84: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
86-86: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
159-159: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
41-41: Column: 1
Hard tabs(MD010, no-hard-tabs)
42-42: Column: 1
Hard tabs(MD010, no-hard-tabs)
43-43: Column: 1
Hard tabs(MD010, no-hard-tabs)
44-44: Column: 1
Hard tabs(MD010, no-hard-tabs)
45-45: Column: 1
Hard tabs(MD010, no-hard-tabs)
46-46: Column: 1
Hard tabs(MD010, no-hard-tabs)
47-47: Column: 1
Hard tabs(MD010, no-hard-tabs)
108-108: Column: 1
Hard tabs(MD010, no-hard-tabs)
109-109: Column: 1
Hard tabs(MD010, no-hard-tabs)
110-110: Column: 1
Hard tabs(MD010, no-hard-tabs)
111-111: Column: 1
Hard tabs(MD010, no-hard-tabs)
112-112: Column: 1
Hard tabs(MD010, no-hard-tabs)
98-98: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
150-150: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
12-12: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
83-83: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
83-83: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
84-84: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
84-84: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
85-85: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
85-85: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
86-86: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
89-89: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
93-93: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
146-146: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
164-164: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
Additional comments not posted (6)
tokens/usecase/pricing/worker/pricing_worker.go (3)
41-41
: LGTM!The asynchronous call to
UpdatePricesSync
using a goroutine is correctly implemented.
Line range hint
44-72
: LGTM!The synchronous price update process is correctly implemented with proper context handling, logging, and error handling.
Line range hint
75-77
: LGTM!The listener registration process is correctly implemented.
domain/pricing.go (1)
123-132
: LGTM!The addition of
UpdatePricesAsync
andUpdatePricesSync
methods to thePricingWorker
interface is correctly implemented.ingest/usecase/ingest_usecase.go (2)
41-44
: LGTM!The addition of the
hasProcessedFirstBlock
atomic flag andfirstBlockWg
wait group to manage the sequencing of block processing is correctly implemented.
107-126
: LGTM!The update to the
ProcessBlockData
method to use the newhasProcessedFirstBlock
flag andfirstBlockWg
wait group is correctly implemented.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Refactor
UpdatePrices
toUpdatePricesSync
for clarity.