Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds Stork as a new price provider to the protocol's oracle system, including a complete price function implementation, configuration registration, test utilities, and framework enhancements to support authentication headers for HTTP requests. Changes
Sequence DiagramsequenceDiagram
participant ExchangeQueryHandler
participant RequestHandler
participant StorkAPI as Stork API<br/>(REST Endpoint)
participant StorkPriceFunction
participant PriceConverter
ExchangeQueryHandler->>ExchangeQueryHandler: Load ExchangeQueryDetails<br/>(includes Headers with auth)
ExchangeQueryHandler->>RequestHandler: GetWithHeaders(ctx, url, headers)
RequestHandler->>RequestHandler: Apply headers to request<br/>(Authorization, etc.)
RequestHandler->>StorkAPI: HTTP GET /v1/prices/latest
StorkAPI-->>RequestHandler: JSON Response<br/>(asset prices scaled 1e18)
RequestHandler-->>ExchangeQueryHandler: *http.Response
ExchangeQueryHandler->>StorkPriceFunction: Parse response
StorkPriceFunction->>PriceConverter: convertStorkPrice(rawPrice)
PriceConverter-->>StorkPriceFunction: Decimal price string
StorkPriceFunction->>StorkPriceFunction: Create StorkTicker instances
StorkPriceFunction->>StorkPriceFunction: Calculate median price<br/>via GetMedianPricesFromTickers
StorkPriceFunction-->>ExchangeQueryHandler: tickerToPrice map
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 1
🧹 Nitpick comments (6)
protocol/daemons/pricefeed/client/constants/static_exchange_query_config_test.go (1)
166-168: Missing a test case for STORK inTestStaticExchangeQueryConfigCache.The length assertion was updated to 16, but the table-driven test
TestStaticExchangeQueryConfigCache(lines 12–163) doesn't include a"Get STORK exchangeDetails"case to validate the STORK entry's values. Every other exchange (except test exchanges) has a corresponding case.Proposed addition to the test table
Add a test case alongside the existing ones (e.g., after the CoinbasePro case at line 150):
"Get Stork exchangeDetails": { exchangeId: exchange_common.EXCHANGE_ID_STORK, expectedValue: &types.ExchangeQueryConfig{ ExchangeId: exchange_common.EXCHANGE_ID_STORK, IntervalMs: 2_000, TimeoutMs: 3_000, MaxQueries: 1, }, expectedFound: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/daemons/pricefeed/client/constants/static_exchange_query_config_test.go` around lines 166 - 168, TestStaticExchangeQueryConfigCache is missing a case for STORK; add a table entry in the existing table-driven test (TestStaticExchangeQueryConfigCache) alongside the other exchange cases (e.g., after the CoinbasePro case) that uses exchange_common.EXCHANGE_ID_STORK and asserts found==true and the expected types.ExchangeQueryConfig values (ExchangeId: exchange_common.EXCHANGE_ID_STORK, IntervalMs: 2_000, TimeoutMs: 3_000, MaxQueries: 1) to validate constants.StaticExchangeQueryConfig contains the STORK entry.protocol/daemons/pricefeed/client/price_function/stork/exchange_query_details_test.go (1)
10-19: Consider adding a test for theHeadersfield.The tests cover
UrlandIsMultiMarketbut skip theHeadersmap, which carries theAuthorizationheader. SinceHeadersis the security-sensitive piece of this integration (authentication), a test asserting its structure (e.g., that the key"Authorization"exists and starts with"Basic ") would improve coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/daemons/pricefeed/client/price_function/stork/exchange_query_details_test.go` around lines 10 - 19, Add a unit test asserting the Authorization header on stork.StorkDetails by creating a new test (e.g., TestStorkHeaders or extend TestStorkIsMultiMarket) that checks stork.StorkDetails.Headers contains the "Authorization" key, that the value is non-empty, and that it starts with the "Basic " prefix; use require.NotEmpty/require.Contains and require.True(strings.HasPrefix(...)) (import strings) to validate structure so the security-sensitive header is covered.protocol/daemons/pricefeed/client/constants/static_exchange_query_config.go (1)
124-130: Minor: Add a documentation link for the Stork rate limit.Other exchange entries include a link to the rate-limit documentation. Consider adding the Stork API docs link for the "5 requests per second" claim for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/daemons/pricefeed/client/constants/static_exchange_query_config.go` around lines 124 - 130, The Stork exchange entry (exchange_common.EXCHANGE_ID_STORK) currently states "rate limit of 5 requests per second" without a reference; add the same style documentation URL comment used for other exchanges above this block that cites Stork's rate-limit docs (e.g., the Stork API or rate-limit page) so the comment for the map entry includes the source link next to the rate-limit claim for consistency with other exchange entries.protocol/daemons/pricefeed/client/types/exchange_query_details.go (1)
26-27: Headers field is well-implemented; API key validation is optional, not critical.The
Headersfield addition is clean and properly handled in the query handler. The handler correctly checkslen(exchangeQueryDetails.Headers) > 0before using headers (line 118 of exchange_query_handler.go).However, if
STORK_API_KEYis unset, theAuthorizationheader becomes"Basic ", which will trigger a 401/403 response from the server. This is not a silent failure—the HTTP error is caught and properly reported through the unavailableMarkets map (lines 159–161). Since docker-compose.yml shows this env var is expected to be configured in deployment, adding explicit validation or logging is optional rather than critical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/daemons/pricefeed/client/types/exchange_query_details.go` around lines 26 - 27, The Authorization header can become "Basic " when STORK_API_KEY is unset which causes needless 401/403 responses; update the header-building logic where exchangeQueryDetails.Headers is applied so you only set the "Authorization" header if the env var / header value is non-empty (i.e., skip adding Authorization when its value is ""), and optionally add a startup check that logs a warning if STORK_API_KEY is unset; look for the Headers field on ExchangeQueryDetails, the code path in the exchange query handler that checks len(exchangeQueryDetails.Headers) and inserts headers, and the unavailableMarkets reporting logic to ensure you still report unavailable markets but avoid inserting empty "Authorization" values.protocol/daemons/pricefeed/client/price_function/stork/stork.go (2)
54-63: Minor: doc example doesn't match actual output.
result.Text('f', 18)always produces 18 decimal places, so"29500000000000000000"would yield"29.500000000000000000", not"29.5"as stated in the comment. Not a bug (downstream parsing handles it), but the doc is misleading.📝 Fix the doc comment
-// For example, "29500000000000000000" becomes "29.5". +// For example, "29500000000000000000" becomes "29.500000000000000000".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/daemons/pricefeed/client/price_function/stork/stork.go` around lines 54 - 63, The doc comment for convertStorkPrice is misleading: result.Text('f', 18) always returns 18 decimal places, so inputs like "29500000000000000000" produce "29.500000000000000000" not "29.5"; update the comment above convertStorkPrice to state that the function returns a decimal string with 18 fractional digits (e.g., "29.500000000000000000") or note that it uses 18 fixed decimal places, referencing convertStorkPrice and the use of result.Text('f', 18) / storkScale so readers know the exact output format.
14-14: Package-level*big.Floatis mutable — consider protecting it.
storkScaleis a shared mutable*big.Float. While current usage as aQuodivisor is safe (it won't be mutated), a future caller could accidentally use it as a receiver (e.g.,storkScale.Quo(...)) and corrupt it for all subsequent calls. A function-scoped value or a getter function would be more defensive.♻️ Option: use a function to return a fresh copy each time
-var storkScale = new(big.Float).SetInt(new(big.Int).Exp(big.NewInt(10), big.NewInt(18), nil)) +func storkScale() *big.Float { + return new(big.Float).SetInt(new(big.Int).Exp(big.NewInt(10), big.NewInt(18), nil)) +}Then update
convertStorkPriceaccordingly:- result := new(big.Float).SetPrec(256).Quo(priceBigFloat, storkScale) + result := new(big.Float).SetPrec(256).Quo(priceBigFloat, storkScale())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/daemons/pricefeed/client/price_function/stork/stork.go` at line 14, storkScale is a package-level mutable *big.Float which is unsafe; change it so each use gets a fresh big.Float instead of sharing the pointer. Replace the package var storkScale with either a small helper function (e.g., storkScale() or getStorkScale()) that constructs and returns a new big.Float with 10^18, or instantiate the scale inside convertStorkPrice before using Quo, and update convertStorkPrice to call the helper or use the local value so no shared *big.Float is mutated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@protocol/daemons/pricefeed/client/price_function/stork/exchange_query_details.go`:
- Around line 10-19: StorkDetails is being created at package init and reads
os.Getenv("STORK_API_KEY") immediately, which can produce a blank "Basic "
Authorization header and prevents runtime key rotation; change this by not
evaluating the env var at package-level: either add a startup validation that
reads os.Getenv("STORK_API_KEY") and fails or logs a warning before constructing
StorkDetails, or replace the package var with a constructor function (e.g.,
GetStorkDetails or NewStorkDetails) that builds the Headers map using the
current env value at startup, or defer injecting the Authorization header inside
the request code that uses StorkPriceFunction; ensure you reference and update
StorkDetails (and any code that depends on it) and add a log warning/error when
the API key is missing.
---
Nitpick comments:
In
`@protocol/daemons/pricefeed/client/constants/static_exchange_query_config_test.go`:
- Around line 166-168: TestStaticExchangeQueryConfigCache is missing a case for
STORK; add a table entry in the existing table-driven test
(TestStaticExchangeQueryConfigCache) alongside the other exchange cases (e.g.,
after the CoinbasePro case) that uses exchange_common.EXCHANGE_ID_STORK and
asserts found==true and the expected types.ExchangeQueryConfig values
(ExchangeId: exchange_common.EXCHANGE_ID_STORK, IntervalMs: 2_000, TimeoutMs:
3_000, MaxQueries: 1) to validate constants.StaticExchangeQueryConfig contains
the STORK entry.
In `@protocol/daemons/pricefeed/client/constants/static_exchange_query_config.go`:
- Around line 124-130: The Stork exchange entry
(exchange_common.EXCHANGE_ID_STORK) currently states "rate limit of 5 requests
per second" without a reference; add the same style documentation URL comment
used for other exchanges above this block that cites Stork's rate-limit docs
(e.g., the Stork API or rate-limit page) so the comment for the map entry
includes the source link next to the rate-limit claim for consistency with other
exchange entries.
In
`@protocol/daemons/pricefeed/client/price_function/stork/exchange_query_details_test.go`:
- Around line 10-19: Add a unit test asserting the Authorization header on
stork.StorkDetails by creating a new test (e.g., TestStorkHeaders or extend
TestStorkIsMultiMarket) that checks stork.StorkDetails.Headers contains the
"Authorization" key, that the value is non-empty, and that it starts with the
"Basic " prefix; use require.NotEmpty/require.Contains and
require.True(strings.HasPrefix(...)) (import strings) to validate structure so
the security-sensitive header is covered.
In `@protocol/daemons/pricefeed/client/price_function/stork/stork.go`:
- Around line 54-63: The doc comment for convertStorkPrice is misleading:
result.Text('f', 18) always returns 18 decimal places, so inputs like
"29500000000000000000" produce "29.500000000000000000" not "29.5"; update the
comment above convertStorkPrice to state that the function returns a decimal
string with 18 fractional digits (e.g., "29.500000000000000000") or note that it
uses 18 fixed decimal places, referencing convertStorkPrice and the use of
result.Text('f', 18) / storkScale so readers know the exact output format.
- Line 14: storkScale is a package-level mutable *big.Float which is unsafe;
change it so each use gets a fresh big.Float instead of sharing the pointer.
Replace the package var storkScale with either a small helper function (e.g.,
storkScale() or getStorkScale()) that constructs and returns a new big.Float
with 10^18, or instantiate the scale inside convertStorkPrice before using Quo,
and update convertStorkPrice to call the helper or use the local value so no
shared *big.Float is mutated.
In `@protocol/daemons/pricefeed/client/types/exchange_query_details.go`:
- Around line 26-27: The Authorization header can become "Basic " when
STORK_API_KEY is unset which causes needless 401/403 responses; update the
header-building logic where exchangeQueryDetails.Headers is applied so you only
set the "Authorization" header if the env var / header value is non-empty (i.e.,
skip adding Authorization when its value is ""), and optionally add a startup
check that logs a warning if STORK_API_KEY is unset; look for the Headers field
on ExchangeQueryDetails, the code path in the exchange query handler that checks
len(exchangeQueryDetails.Headers) and inserts headers, and the
unavailableMarkets reporting logic to ensure you still report unavailable
markets but avoid inserting empty "Authorization" values.
Comment on lines
+10
to
+19
| var ( | ||
| StorkDetails = types.ExchangeQueryDetails{ | ||
| Exchange: exchange_common.EXCHANGE_ID_STORK, | ||
| Url: "https://rest.jp.stork-oracle.network/v1/prices/latest?assets=$", | ||
| PriceFunction: StorkPriceFunction, | ||
| IsMultiMarket: true, | ||
| Headers: map[string]string{ | ||
| "Authorization": "Basic " + os.Getenv("STORK_API_KEY"), | ||
| }, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for os.Getenv usage in exchange details files
find . -path "*/price_function/*/exchange_query_details.go" -type f 2>/dev/null | head -20Repository: dydxprotocol/v4-chain
Length of output: 1454
🏁 Script executed:
#!/bin/bash
# Check for os.Getenv or os.Environ usage across price_function directory
rg -n 'os\.(Getenv|Environ)' --type go -g '*/price_function/*' -B 2 -A 2Repository: dydxprotocol/v4-chain
Length of output: 213
🏁 Script executed:
#!/bin/bash
# Examine the stork exchange_query_details.go file to understand the structure
cat -n "protocol/daemons/pricefeed/client/price_function/stork/exchange_query_details.go"Repository: dydxprotocol/v4-chain
Length of output: 754
🏁 Script executed:
#!/bin/bash
# Search for os.Getenv across all exchange_query_details.go files more directly
rg -n 'os\.Getenv' protocol/daemons/pricefeed/client/price_function/ -B 2 -A 2Repository: dydxprotocol/v4-chain
Length of output: 604
🏁 Script executed:
#!/bin/bash
# Check a few other exchange files to see if they use Headers with auth
for file in protocol/daemons/pricefeed/client/price_function/coinbase_pro/exchange_query_details.go \
protocol/daemons/pricefeed/client/price_function/binance/exchange_query_details.go \
protocol/daemons/pricefeed/client/price_function/kraken/exchange_query_details.go; do
echo "=== $file ==="
cat -n "$file" | head -30
doneRepository: dydxprotocol/v4-chain
Length of output: 2241
🏁 Script executed:
#!/bin/bash
# Look for any startup validation or key initialization logic in stork package
find protocol/daemons/pricefeed/client/price_function/stork -type f -name "*.go" | xargs cat | head -100Repository: dydxprotocol/v4-chain
Length of output: 2915
os.Getenv("STORK_API_KEY") is evaluated once at package initialization time.
StorkDetails is a package-level var, so the environment variable is read when the package is first imported. If the env var is unset, the Authorization header becomes "Basic " (empty credential), and requests will be sent with this malformed header. Consider adding startup-time validation to ensure the key is present and log a warning if it's missing.
Additionally, the API key cannot be rotated without restarting the process — acceptable if that matches your operational model, but worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@protocol/daemons/pricefeed/client/price_function/stork/exchange_query_details.go`
around lines 10 - 19, StorkDetails is being created at package init and reads
os.Getenv("STORK_API_KEY") immediately, which can produce a blank "Basic "
Authorization header and prevents runtime key rotation; change this by not
evaluating the env var at package-level: either add a startup validation that
reads os.Getenv("STORK_API_KEY") and fails or logs a warning before constructing
StorkDetails, or replace the package var with a constructor function (e.g.,
GetStorkDetails or NewStorkDetails) that builds the Headers map using the
current env value at startup, or defer injecting the Authorization header inside
the request code that uses StorkPriceFunction; ensure you reference and update
StorkDetails (and any code that depends on it) and add a log warning/error when
the API key is missing.
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
1 participant
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.
Changelist
Added Stork support with 2 assets: SPXUSD and XAGUSD
Test Plan
Ran tests + built the chain locally
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Release Notes