WIP - Pair Pricing logic & other changes for HYPE support #3532
Conversation
parodime
requested review from
dwasse,
trajan0x and
aureliusbtc
as code owners
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR implements multiple incremental updates across various components. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PriceFetcher as CoingeckoPriceFetcher
participant Cache
participant API as CoinGeckoAPI
Client->>PriceFetcher: GetPrice(token)
PriceFetcher->>Cache: Check cachedPrice(token)
alt Cached data valid
Cache-->>PriceFetcher: Return cachedPrice
PriceFetcher-->>Client: Return cachedPrice.value
else No valid cache
PriceFetcher->>API: Request price for token
API-->>PriceFetcher: Return price
PriceFetcher->>Cache: Update cache(token, price)
PriceFetcher-->>Client: Return price
end
sequenceDiagram
participant Requester
participant FeePricer
participant PriceFetcher
Requester->>FeePricer: PricePair(ctx, stepLabel, baseTokenChain, pricedTokenChain, baseToken, pricedToken, baseValueWei)
FeePricer->>PriceFetcher: GetPrice(baseToken)
PriceFetcher-->>FeePricer: Return baseTokenPrice
FeePricer->>PriceFetcher: GetPrice(pricedToken)
PriceFetcher-->>FeePricer: Return pricedTokenPrice
FeePricer->>FeePricer: Calculate conversion ratio and values
FeePricer-->>Requester: Return PricedValuePair (base and priced token values)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3532 +/- ##
===================================================
+ Coverage 11.02226% 19.34597% +8.32371%
===================================================
Files 259 508 +249
Lines 29513 46206 +16693
===================================================
+ Hits 3253 8939 +5686
- Misses 25792 36231 +10439
- Partials 468 1036 +568
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ectUSD. Impvs debug output.
Deploying sanguine-fe with
|
| Latest commit: |
f9a737f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5f2ba128.sanguine-fe.pages.dev |
| Branch Preview URL: | https://ft-hype-rfq.sanguine-fe.pages.dev |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (30)
services/rfq/api/client/client.go (3)
146-149: Improved error handling for unauthorized PutQuote requestsThe added error handling for HTTP 400 status code provides better feedback when a quote operation fails due to authorization issues. This prevents silent failures and makes debugging easier.
However, the comment is slightly misleading as the code now returns an error rather than panicking.
Consider improving the comment and error message for more clarity:
- // this will panic. typically indicates no authorization to quote + // HTTP 400 typically indicates authorization issues when trying to quote if res.RawResponse.StatusCode == 400 { - return fmt.Errorf("PutQuote Status code: %d", res.RawResponse.StatusCode) + return fmt.Errorf("PutQuote failed with status code %d: authorization error", res.RawResponse.StatusCode) }
164-167: Improved error handling for unauthorized PutBulkQuotes requestsThe added error handling for HTTP 400 status code provides better feedback when bulk quote operations fail due to authorization issues. This prevents silent failures and makes debugging easier.
However, the comment is slightly misleading as the code now returns an error rather than panicking.
Consider improving the comment and error message for more clarity:
- // this will panic. typically indicates no authorization to quote + // HTTP 400 typically indicates authorization issues when trying to quote if res.RawResponse.StatusCode == 400 { - return fmt.Errorf("PutBulkQuotes Status code: %d", res.RawResponse.StatusCode) + return fmt.Errorf("PutBulkQuotes failed with status code %d: authorization error", res.RawResponse.StatusCode) }
143-145: Consider handling response data for future extensibilityCurrently, the response data is ignored with
_ = resafter a TODO comment. While this might be sufficient now, it could be helpful to extract and log any response body content for debugging purposes, especially for error cases.Consider adding response body logging for error cases:
// TODO: Figure out if there's anything to do with the response, right now it's result: Status Code 200 OK - _ = res + // Keep response for potential future use and for error handling // HTTP 400 typically indicates authorization issues when trying to quote if res.RawResponse.StatusCode == 400 { + body := string(res.Body()) + logger.Debugf("PutQuote failed with status code %d: %s", res.RawResponse.StatusCode, body) return fmt.Errorf("PutQuote failed with status code %d: authorization error", res.RawResponse.StatusCode) }Also applies to: 161-163
services/rfq/relayer/relconfig/config.go (1)
52-55: Good addition of CoinGecko API configuration.The addition of CoinGecko API key and URL fields to the Config struct will allow for more flexible configuration, especially for testing purposes.
Consider standardizing the naming convention between the two fields - one uses "Api" while the other uses "API".
- // CoinGeckoApiKey is the CoinGecko API key. - CoinGeckoApiKey string `yaml:"coingecko_api_key"` + // CoinGeckoAPIKey is the CoinGecko API key. + CoinGeckoAPIKey string `yaml:"coingecko_api_key"`services/rfq/relayer/pricer/fee_pricer_test.go (1)
33-98: Comprehensive test coverage for the PricePair method.This is an excellent set of test cases covering multiple token pair conversions including:
- ETH/USDC
- ETH/MATIC
- ETH/DirectUSD
- ETH/BNB and back
- ETH/BTC and back
- Testing with a large ETH amount
Good attention to testing precision loss in bidirectional conversions.
Consider adding cases that test error handling, such as:
- Invalid token names
- Price fetch failures
- Edge cases like zero amounts or extremely large amounts
Add a test case for error handling:
// Test with invalid token name fee, err := feePricer.PricePair(s.GetTestContext(), "Invalid token case", s.origin, s.destination, "INVALID_TOKEN", "USDC", *baseValueWei) s.Error(err) s.Nil(fee)services/rfq/e2e/setup_test.go (1)
277-344: Well-structured mock server implementation with room for improvement.The mock HTTP handler is well organized with proper error handling. However, there are opportunities to enhance it:
- The hardcoded prices make the mock less flexible for diverse testing scenarios
- The logic for checking coin IDs could be simplified
- Error logging only appears in test logs but doesn't cause test failures
Consider making the prices configurable:
+type MockPriceData map[string]float64 +// Initialize with default prices that can be overridden in tests +var defaultMockPrices = MockPriceData{ + "ethereum": 2000.0, + "usd-coin": 1.0, + "berachain-bera": 5.0, + "hyperliquid": 10.0, +} +func (i *IntegrationSuite) setupMockCoinGeckoServer(customPrices ...MockPriceData) { + // Merge custom prices with defaults if provided + mockPrices := defaultMockPrices + if len(customPrices) > 0 { + for k, v := range customPrices[0] { + mockPrices[k] = v + } + }Then use
mockPrices[coinID]instead of the switch statement.services/rfq/relayer/quoter/quoter_test.go (7)
6-6: Consider using test-specific environment handling.
Using the globalospackage for environment variables within tests can introduce side effects across multiple tests. Consider usingt.Setenvor resetting the environment afterward to ensure better isolation.
24-25: Reset or revert environment variable between tests.
SettingdebugOutputcan pollute subsequent tests if not reverted. Ensure each test case starts from a clean state to avoid unexpected interactions.
75-99: Include corner case inputs.
While this test checks typical BNB → BTC quoting, try adding scenarios with minimal or very large balances to confirm the quoting logic handles extremes properly.
101-125: Validate intermediate steps in gas-to-token quoting.
The final BNB fee is asserted, but intermediate calculations (like partial rounding) are not checked. Adding such checks could catch subtle rounding errors.
127-129: Minimize test side effects by cleaning environment.
Use a teardown or test function scope for environment changes to avoid lingering debug flags.
194-230: Extend coverage on decimal mismatches and token configurations.
The test exercises some mismatch scenarios. Additional boundary checks (e.g. extremely large decimals or unsupported tokens) can boost confidence in correctness.
291-531: Refactor lengthy test for maintainability.
This large test covers many parameter sets in a single function. Adopting a table-driven approach or splitting into multiple smaller tests could improve readability.services/rfq/relayer/pricer/fee_pricer.go (4)
8-12: Isolate environment-based debug logic.
Imports of"os"and"strings"are used for runtime debug toggling. Consider a dedicated debug configuration or CLI flag to avoid environment-related fragility in production.
54-67: Clear struct definitions for priced data.
Breaking token data intoTokenValueandPricedValuePairis intuitive. Consider adding small doc comments on each field for maintainability.
209-348: Refine debug logging approach.
Relying on string matching ofos.Getenv("debugOutput")can be brittle. A structured approach (e.g. a logger or a config parameter) typically provides more flexibility and consistency.
470-471: Ensure coherence between config-based and fetched token prices.
This code returns a price from the config, whereas the main path usespriceFetcher. Confirm both remain in sync or clarify the expected fallback logic.services/rfq/relayer/pricer/fetcher.go (7)
35-40: Consider separating TTL from cached data
ThecachedPricestruct elegantly represents price data with its TTL. However, placing time-based logic and data in the same struct can sometimes hinder clarity. You might consider a dedicated struct or helper for cache expiration checks if this grows more complex.
157-161: Handling “DirectUSD” as a special token
Returning a fixed1.0for"DirectUSD"can simplify referencing a direct USD price. Keep an eye on future expansions in case more “direct” or synthetic tokens are needed, so your code remains flexible.
173-173: Non-cached fallback
Markingused_cached_priceas false in the span is straightforward. Consider including the token as an attribute here for improved telemetry correlation when cache retrieval misses occur frequently.
212-212: Enhance error message with more context
fmt.Errorf("coingecko stat code: %v", r.Status)is fine, but adding the queried token or URL can help diagnose issues if the external request fails repeatedly.
215-217: Consider logging body close error
ThecloseErris ignored. Logging or handling this error can help identify underlying issues (e.g., network or resource leaks).
237-256: Unfinished secondary price handling
This block hints at fetching another price source and enforcing a 5% difference threshold. Currently, it’s partially stubbed out. Finalize or remove the placeholder logic to prevent confusion.
278-280: PlaceholderGetSecondaryPrice
Returning1, nilis a valid placeholder but might cause confusion if overlooked. Clarify with additional comments or remove until properly implemented.services/rfq/relayer/quoter/quoter.go (6)
610-616: Logging errors for fee retrieval
Calls tologger.Errorbefore returning the wrapped error is a good approach. Validate that these logs won’t be excessively noisy if the same error triggers repeatedly.
620-621: Logging “GetRfqAddress” errors
Again, re-logging the error before returning a wrapped version helps with immediate debugging. Confirm that the logger includes relevant chain ID or token data.
639-677: Verbose debug printing
Thefmt.Printfblock under thegeneratequotedebug flag can be invaluable for dev. Consider a structured logging approach in the future if you need to parse or store logs externally.
734-736:getOriginAmountfunction expansion
This new method centralizes the logic for computing a safe and practical origin amount to quote. Given the complexity, maintain robust unit tests for each offset step, gas check, and final clip operation. Keep an eye on cyclomatic complexity as more conditions are introduced.Also applies to: 737-748
873-879: Repricing back to origin
Double re-pricing is conceptually valid but can be fragile if any new fees or offsets are introduced. Keep track of performance overhead with repeated PricePair calls.
933-933: Offset calculations for destination amounts
Combining offsets for origin offset, destination offset, and quote width is a flexible approach. Keep thorough documentation of how each offset is intended so future maintainers understand the layered math.Also applies to: 935-935, 955-955
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
ethergo/go.sumis excluded by!**/*.sumservices/cctp-relayer/go.sumis excluded by!**/*.sumservices/explorer/go.sumis excluded by!**/*.sumservices/omnirpc/go.sumis excluded by!**/*.sumservices/rfq/go.sumis excluded by!**/*.sumservices/scribe/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.gitignore(1 hunks).golangci-version(1 hunks).golangci.yml(4 hunks)agents/domains/evm/summit.go(1 hunks)ethergo/go.mod(1 hunks)ethergo/listener/listener.go(1 hunks)services/rfq/api/client/client.go(2 hunks)services/rfq/e2e/rfq_test.go(4 hunks)services/rfq/e2e/setup_test.go(4 hunks)services/rfq/relayer/pricer/fee_pricer.go(8 hunks)services/rfq/relayer/pricer/fee_pricer_test.go(1 hunks)services/rfq/relayer/pricer/fetcher.go(5 hunks)services/rfq/relayer/quoter/quoter.go(9 hunks)services/rfq/relayer/quoter/quoter_test.go(9 hunks)services/rfq/relayer/quoter/suite_test.go(5 hunks)services/rfq/relayer/relconfig/config.go(1 hunks)services/rfq/relayer/relconfig/getters.go(3 hunks)services/rfq/relayer/service/relayer.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (57)
.golangci-version (1)
1-1: Upgraded GolangCI linter versionThe update from 1.61.0 to 1.64.5 aligns with the changes made to the linter configurations in
.golangci.yml. This upgrade brings the project up to date with the latest linter features and bug fixes.agents/domains/evm/summit.go (1)
126-126: Fixed linter directive formatThe comment directive has been updated from the incorrect
//nolint:nil,nilformat to the correct//nolint:nilnilformat. This change maintains consistent linter directive syntax across the codebase..golangci.yml (6)
8-8: Simplified govet configurationThe govet linter configuration has been simplified from explicit settings to use default behavior. This approach reduces configuration complexity while maintaining linting capability.
53-54: Updated linter name to match upstream changesThe linter has been renamed from
goerr113toerr113to reflect changes in the upstream linter. This ensures compatibility with the newer GolangCI-lint version.
60-60: Updated linter name to match upstream changesThe linter has been renamed from
gomndtomndto reflect changes in the upstream linter. This ensures compatibility with the newer GolangCI-lint version.
86-87: Replaced deprecated linter with new alternativeThe change replaces the deprecated
tenvlinter withusetestingin the disabled list. This keeps the configuration aligned with the latest linter capabilities.
91-101: Restructured exclusion configurationThe configuration has been improved by:
- Setting
exclude-dirs-use-defaultto false- Explicitly listing directories and file patterns to exclude
- Moving from
skip-dirsandskip-filesto the more standardizedexclude-dirsandexclude-filesThis makes the configuration more explicit and maintainable.
142-142: Updated base revision for lintingThe
new-from-revvalue has been updated to a more recent commit hash. This ensures linting checks are compared against a current baseline.services/rfq/api/client/client.go (1)
96-112:✅ Verification successful
Verify authentication token expiration handling
The authentication mechanism uses the current Unix timestamp for signing. Ensure there's adequate handling for timestamp validation on the server-side to prevent replay attacks while allowing for minor clock skew between client and server.
🏁 Script executed:
#!/bin/bash # Check if there are any validation checks for token expiration in the server implementation echo "Looking for timestamp validation in server code:" rg -A5 "strconv.Itoa\(int\(time.Now\(\).Unix\(\)\)\)" --type go rg -A10 -B3 "Authorization.*Header" --type go | grep -i "timestamp\|expir\|valid"Length of output: 1814
Authentication Token Expiration Handling Verified
The client-side code correctly generates the timestamp and signs the request, and our review of the server code (in services/rfq/api/rest/auth.go) confirms that it already includes a check to ensure the timestamp isn’t older than a given deadline. Please double-check that the server’s logic explicitly accounts for minor clock skew so that valid requests aren’t inadvertently rejected.
.gitignore (1)
137-142: Good security practice for sensitive configuration files.These gitignore rules will prevent accidentally committing API keys and other sensitive configuration values, while still tracking the template file.
The exclusion of config files except for the template is a good practice since it allows developers to share the structure without sharing private credentials.
ethergo/listener/listener.go (1)
90-90: Verify impact of reducing maxGetLogsRange from 2000 to 50.This is a significant reduction (97.5%) in the maximum range of blocks that can be fetched in a single call. This change will cause the listener to make more frequent RPC calls with smaller ranges, which could impact performance and RPC usage limits.
A previous review comment suggested applying this change only to chainID 999, but this implementation applies it globally. Please verify that this won't negatively impact production performance on other chains.
#!/bin/bash # Description: Check if there's conditional handling for different chain IDs in the listener code # Look for any chain ID conditions in the listener files echo "Searching for chain ID conditions in the listener package:" rg -A 3 -B 3 "chainID\s*==\s*999|c\.chainID\s*==\s*999" --glob "ethergo/listener/**/*.go" # Check for performance metrics or logs related to log fetching echo "Searching for any performance metrics for log fetching:" rg -A 3 "getlog|get_log|fetch_log|log range|log batch" --glob "ethergo/**/*.go"services/rfq/relayer/quoter/suite_test.go (3)
58-69: Good update to use specific token addresses in tests.Using consistent Ethereum addresses for tokens rather than empty strings improves the test's realism and ensures more comprehensive testing coverage.
130-132: Expanded token pair support for BNB and BTC.The addition of BNB to BTC quotable token pairs is good for testing price pair functionality. This change will help ensure proper cross-token pricing.
145-151:✅ Verification successful
Enhanced price fetcher mock configuration.
Adding mock price responses for all tokens including the new "HYPE" token ensures comprehensive test coverage for the pricing functionality.
Are there tests for these new token pairings? Let's check if the related test files contain tests that utilize these new token pair configurations:
🏁 Script executed:
#!/bin/bash # Description: Verify if tests exist for new token pairings like BNB-BTC and HYPE # Search for tests involving BNB, BTC pair testing echo "Searching for BNB/BTC pair tests:" rg -A 10 "BNB.*BTC|BTC.*BNB" --glob "services/rfq/relayer/**/*_test.go" # Search for tests involving the HYPE token echo "Searching for HYPE token tests:" rg "HYPE" --glob "services/rfq/relayer/**/*_test.go" # Search for any PricePair tests that might use these tokens echo "Searching for PricePair tests:" rg "PricePair|Price.*Pair" --glob "services/rfq/relayer/**/*_test.go"Length of output: 6479
Enhanced price fetcher mock configuration verified — tests for the new token pairings are present.
The addition of the "HYPE" token mock (along with the other tokens) is covered by existing tests. Specifically:
- BNB/BTC Pairing: Tests in services/rfq/relayer/quoter/suite_test.go and referenced in services/rfq/relayer/pricer/fee_pricer_test.go confirm that the pairing (e.g., BNB >> BTC) is correctly exercised.
- HYPE Token: Grep results show that the "HYPE" token is utilized in tests from services/rfq/relayer/quoter/suite_test.go, quoter_test.go, and fee_pricer_test.go.
- PricePair Coverage: Additional tests in fee pricers use the updated token configurations to verify pricing for various pairs.
The mock configuration snippet in question is:
priceFetcher.On(testsuite.GetFunctionName(priceFetcher.GetPrice), mock.Anything, "USDC").Return(1., nil) priceFetcher.On(testsuite.GetFunctionName(priceFetcher.GetPrice), mock.Anything, "DirectUSD").Return(1., nil) priceFetcher.On(testsuite.GetFunctionName(priceFetcher.GetPrice), mock.Anything, "ETH").Return(2000., nil) priceFetcher.On(testsuite.GetFunctionName(priceFetcher.GetPrice), mock.Anything, "MATIC").Return(0.5, nil) priceFetcher.On(testsuite.GetFunctionName(priceFetcher.GetPrice), mock.Anything, "BTC").Return(95000., nil) priceFetcher.On(testsuite.GetFunctionName(priceFetcher.GetPrice), mock.Anything, "BNB").Return(600., nil) priceFetcher.On(testsuite.GetFunctionName(priceFetcher.GetPrice), mock.Anything, "HYPE").Return(15., nil)Given that tests covering these configurations are in place, this enhancement can be approved.
services/rfq/e2e/rfq_test.go (4)
54-55: Clean declaration of mock server component.Good structure for testing - explicitly declaring the mock component in the suite.
82-87: Consider adding more token-to-id mappings for thorough testing.The token-to-id mapping is a good start, but as you add more token support, consider expanding this mapping to ensure complete test coverage.
[good_practice]
94-95: Proper sequencing of mock server setup.Excellent placement - setting up the mock server before backends ensures the relayer will use the mock instead of making real API calls.
106-115:❓ Verification inconclusive
Thorough cleanup in TearDownTest.
Good resource management by properly closing the mock server and resetting the CoinGecko map. This prevents test interference and resource leaks.
However, be cautious with
UnsafeResetCoingeckoMap()- this suggests modifying global state which could cause issues if tests run in parallel.
🏁 Script executed:
#!/bin/bash # Check if tests use parallel testing that could lead to race conditions with global state # Look for test parallelism flags or settings grep -r "t.Parallel()" --include="*_test.go" . grep -r "go test.*-parallel" .Length of output: 2575
Global State Cleanup is Solid, but Caution is Needed for Parallel Tests
- The teardown logic correctly closes the mock server and resets the CoinGecko map, which helps avoid resource leaks and test interference.
- However, since
UnsafeResetCoingeckoMap()manipulates global state and several tests in the repository are marked witht.Parallel(), please verify that tests dependent on CoinGecko’s state are not affected by concurrent executions. Consider isolating this global state change or adding proper synchronization where necessary.services/rfq/relayer/service/relayer.go (1)
130-130:❓ Verification inconclusive
Enhanced configuration for the CoinGecko price fetcher.
Good improvement passing the full config object to the price fetcher instead of just the timeout. This allows access to additional configuration options like
CoinGeckoAPIURLandCoinGeckoApiKey, improving modularity and configurability.Ensure the
NewCoingeckoPriceFetcherimplementation properly handles all the configuration parameters from the config object.
🏁 Script executed:
#!/bin/bash # Check the implementation of NewCoingeckoPriceFetcher to ensure it properly utilizes the config parameters # Find the implementation grep -r "func NewCoingeckoPriceFetcher" --include="*.go" ./services/rfq/Length of output: 419
Enhanced configuration for the CoinGecko price fetcher – Verification Needed
Good improvement on passing the full config object to the price fetcher instead of only the timeout. The grep results confirm that the implementation in
services/rfq/relayer/pricer/fetcher.gonow accepts the configuration (relConfig relconfig.Config), allowing for additional parameters likeCoinGeckoAPIURLandCoinGeckoApiKey. However, please verify that the implementation actually extracts and uses these parameters appropriately within the function body.
- Verify that
CoinGeckoAPIURLandCoinGeckoApiKey(and any other relevant options) are properly accessed from therelConfigobject.- Double-check the usage in the
NewCoingeckoPriceFetcherimplementation inservices/rfq/relayer/pricer/fetcher.go.services/rfq/relayer/pricer/fee_pricer_test.go (1)
45-45: Added support for the new HYPE token.This addition aligns with the PR title mentioning "HYPE support".
services/rfq/e2e/setup_test.go (3)
346-351: Ensure proper server initialization and logging.The server initialization is correct and includes useful logging. The global struct field allows the server to be accessed from other test methods.
361-366: Proper conditional inclusion of mock API URL.Good practice checking if the mock server exists before adding its URL to the configuration. This prevents configuration errors when the mock is not running.
429-429: Configuration integration for CoinGeckoAPIURL.This completes the integration by passing the mock server URL to the relayer configuration.
services/rfq/relayer/quoter/quoter_test.go (1)
49-73: Broaden test scenarios.
The test covers ETH → MATIC with a final amount check. Consider adding edge cases (e.g. zero gas) or additional assertions (like verifying that no unexpected fields are set) to further strengthen coverage.services/rfq/relayer/relconfig/getters.go (3)
399-400: Clarify the difference between “asset not on chain” and “zero offset.”
Returning0.0here can mask a missing token configuration. Ensure the calling code correctly interprets a zero offset vs. an unknown token in order to avoid silent misconfiguration.
686-691: Confirm the 5-decimal default for “DirectUSD.”
Hardcoding 5 decimals may not match real stablecoin conventions. Validate this aligns with your business logic or the intended usage of “DirectUSD.”
716-719: Zero-address safeguard is a solid addition.
Explicitly failing for0x0000...addresses prevents invalid references and potential mishandling. Good catch.services/rfq/relayer/pricer/fee_pricer.go (2)
35-36: Adding PricePair method is beneficial.
This method centralizes cross-chain token pricing. Ensure upstream handles errors gracefully for unrecognized tokens or chains.
350-405: Double-check fee rounding methodology.
The final fee is truncated by converting the float toInt(). If partial fees are meaningful, rounding strategies might need broader consideration (e.g., always rounding up for safety).services/rfq/relayer/pricer/fetcher.go (13)
7-7: Consolidate imports if appropriate
These newly introduced imports foros,sync, andrelconfigappear necessary for environment checks, synchronization locks, and configuration usage. If the usage pattern continues to grow, consider grouping or aliasing them for clarity, though it's not mandatory at this time.Also applies to: 10-10, 11-11, 16-16
30-32: Initialize all struct fields in constructor
Fieldshandler,client, andrelConfigare added for coherent grouping of dependencies. Ensure that all fields (especially new ones) are initialized in the constructor to avoid nil references.
42-48: Global mutable state requires careful synchronization
DefiningglobalCache,cacheMu, andcoingeckoMuas package-level variables centralizes caching logic but risks accidental misuse. The mutex locking approach is correct for read/write concurrency, but be vigilant in verifying that all cache interactions consistently acquire the appropriate lock.
51-51: Constructor with dynamic configuration
AcceptingrelConfiginNewCoingeckoPriceFetcherand storing it inrelConfig *relconfig.Configfosters runtime flexibility. Validate that downstream calls reference the loaded config only through this field, and confirm that concurrency is not an issue when modifyingrelConfigoutside of test environments.Also applies to: 59-61
65-65: Updated token mappings for Coingecko
Entries for"USDC": "usd-coin","BERA": "berachain-bera", and"HYPE": "hyperliquid"look correct. Ensure your tests cover all newly added tokens to confirm the relayer picks them up properly.Also applies to: 69-70
72-74: Global copy for default Coingecko map
Preserving the original map inoriginalCoingeckoIDLookupensures test manipulations can be reversed. Good approach for test isolation.
75-81: Initialization of original map
Storing original values ofcoingeckoIDLookupat init-time is a straightforward tactic. Confirm that no concurrent code modifies the map beforeinit()finishes in multi-file or multi-package test harnesses.
83-101:UnsafeGetCoingeckoIDMapin test contexts
This method prohibits calls unless in a test environment. The RLock usage is correct for reading the map. Make sure calling code robustly handles the possible error message if invoked outside test mode.
122-144: Reset logic consistency
UnsafeResetCoingeckoMapsimilarly restricts usage to test environments, restoring the original map. Ensure that this always runs after test updates to avoid leftover changes for subsequent tests and fully revert to the known default state.
163-171: Cached price retrieval
The lock usage withcacheMuis appropriate. The code checks if the cached entry is still valid by comparingtime.Since(cached.timestamp) < cached.timeToLive. Be sure to handle scenarios wheretimeToLivemight be zero or negative unexpectedly.
175-175: Read-lock usage
Lock usage around thecoingeckoIDLookupmap read is correct. This ensures thread-safe lookups. Continue to maintain a consistent pattern of RLock/RUnlock for read paths and Lock/Unlock for writes.Also applies to: 177-177
183-200: Selective use of official vs. custom CoinGecko endpoints
The fallback logic forc.relConfig.CoinGeckoAPIURLandapiKeyis sensible, especially controlling the addition ofx_cg_pro_api_keyonly for the official endpoint. Confirm your QA tests cover both official and custom endpoints.
258-274: Successfully populating global cache
This section properly lockscacheMubefore writing the newcachedPriceentry with a chosenttl. The approach seems correct; keep an eye on whether the TTL should be configurable for different tokens beyond “USDC” and everything else.services/rfq/relayer/quoter/quoter.go (15)
10-10: New import usage
The additional"os"import is utilized for environment-specific logging blocks (e.g., debug output). This is acceptable; keep your import usage consistent with the environment checks performed.
596-602: Enforcing zero quote when gas threshold is surpassed
IngenerateQuote, ifgetOriginAmountreturns an error indicating the min gas token exceeds the quote amount, you setmaxQuoteAmountOriginto zero. This is a practical fallback; just ensure callers properly handle a zero quote scenario.
624-630: PricePair logic
During final origin–destination reprice, the inlinePricePaircall is labeled “4 - Final Dest.” Make sure these debug labels remain consistent and updated if new reprice steps are introduced.
632-636: Destination amount retrieval
You retrieve the final destination amount by callingm.getDestAmount. If errors occur, you log them withlogger.Error. Ensure test coverage handles both success and error paths thoroughly.
679-682: Safety check to prevent inflated destination
Rejecting a scenario in whichdestAmountPriced.PricedToken.UsdexceedsmaxQuoteAmount.BaseToken.Usdis a wise fail-safe. Keep verifying no edge cases remain—like fractional differences near zero.
690-690: Use ofmaxQuoteAmountOrigin.String()
Setting theMaxOriginAmountfield tomaxQuoteAmountOrigin.String()ensures clarity in the final quote struct. Just confirm numeric conversions remain consistent across the entire pipeline (decimals, big.Int usage, etc.).
775-776: Missing or incomplete snippet
These lines appear indicated but are not fully shown in the code snippet. Verify no partial changes for quote percentage logic. If actual modifications exist here, ensure thorough coverage.
817-818: Decreasing quote amount when maximum balance is reached
Clamping the quote to zero ifquotableBalance<= zero is correct. Helps avoid negative quotes. This code chunk is consistent with similar safety checks.
829-832: Repricing back to destination
Here you reprice from origin to destination token, then clamp if it exceeds the actualinput.DestBalance. This layered approach is logical but can get complex. Unit tests for each clamp step are important.Also applies to: 833-839
855-856: Max relay amount on destination
ApplyingGetMaxRelayAmountafter you do the reprice ensures you never exceed your configured thresholds. The debug events are helpful for investigating.Also applies to: 857-861
864-872: Gas reserve deduction
m.deductGasReserveensures you retain minimal gas. You handle errors if the resulting amount is negative. That’s crucial to avoid quoting a token balance that compromises operational gas.
884-891: Exact deposit coverage
Ifinput.OriginAmountExactis non-nil, you clamp the final quote to match it or return zero. Good user-oriented approach for supporting exact deposit flows.
895-895: Return final quote amount
ReturningquoteAmountOriginneatly concludes the multi-step calculation. Check alignment with the rest of the code to ensure it’s not overwritten.
969-979: Ensure float arithmetic does not cause silent rounding discrepancies
InapplyOffset, factoring byoffsetBpsinvolves floating operations. Even though you reconvert tobig.Int, slight rounding can occur. That might be fine for quotes if you confirm acceptance criteria for partial rounding.
995-1008: Bulk quote submissions
Submissions viaPutBulkQuotesare likely more efficient than iterative calls. Validate that the server’s rate-limits or concurrency constraints are respected if the quote volume grows large.
| @@ -85,6 +85,7 @@ require ( | |||
| cloud.google.com/go/longrunning v0.5.7 // indirect | |||
| dario.cat/mergo v1.0.0 // indirect | |||
| github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect | |||
| github.com/BurntSushi/toml v1.4.0 // indirect | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Added TOML parsing dependency
The addition of github.com/BurntSushi/toml v1.4.0 as an indirect dependency indicates that TOML configuration parsing capabilities are now being utilized somewhere in the dependency tree. This is a widely used, well-maintained library for TOML parsing.
🏁 Script executed:
#!/bin/bash
# Check if any files in the repository directly import the TOML library
echo "Checking for direct imports of github.com/BurntSushi/toml:"
rg -l "github.com/BurntSushi/toml" --type goLength of output: 198
Clarify TOML Dependency Usage in go.mod
The dependency github.com/BurntSushi/toml v1.4.0 in the ethergo/go.mod file is marked as an indirect dependency. However, our search found a direct import in agents/internal/require.go, indicating that TOML parsing is used directly in the codebase.
- Please verify if the direct import in
agents/internal/require.gois intentional. - If it is meant to be a direct usage, update the go.mod file to remove the
// indirectannotation so that the dependency is declared as direct.
| err := os.Setenv("GO_ENVIRONMENT", "test") | ||
| i.NoError(err) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid modifying global environment variables in tests.
Setting environment variables can interfere with other tests running in parallel. Consider using a local configuration approach instead of modifying global state.
-// Set test environment
-err := os.Setenv("GO_ENVIRONMENT", "test")
-i.NoError(err)
+// Use local configuration or context for test environment
+testContext := context.WithValue(i.GetTestContext(), "environment", "test")Committable suggestion skipped: line range outside the PR's diff.
| // UnsafeUpdateCoingeckoMap updates the coingeckoIDLookup map with the provided entries. | ||
| // This method is only meant to be used in test environments and will return an error if used in production. | ||
| func UnsafeUpdateCoingeckoMap(entries map[string]string) error { | ||
| // Only allow updates in test environments | ||
| if !(os.Getenv("GO_ENVIRONMENT") != "test" || os.Getenv("GO_ENV") != "test" || os.Getenv("ENVIRONMENT") != "test" || core.HasEnv("CI")) { | ||
| panic("UnsafeUpdateCoingeckoMap can only be called in test environments") | ||
| } | ||
|
|
||
| coingeckoMu.Lock() | ||
| defer coingeckoMu.Unlock() | ||
|
|
||
| // Update the map with new entries | ||
| for token, coinGeckoID := range entries { | ||
| coingeckoIDLookup[token] = coinGeckoID | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential logical confusion in environment checks
The environment check in UnsafeUpdateCoingeckoMap includes if !(os.Getenv("GO_ENVIRONMENT") != "test" || os.Getenv("GO_ENV") != "test" || os.Getenv("ENVIRONMENT") != "test" || core.HasEnv("CI")). This condition is somewhat convoluted and might not correctly capture “test OR CI” use cases. Revisit to ensure it truly permits updates only in valid test/CI scenarios.
- if !(os.Getenv("GO_ENVIRONMENT") != "test" || os.Getenv("GO_ENV") != "test" || os.Getenv("ENVIRONMENT") != "test" || core.HasEnv("CI")) {
+ // Example: Ensure environment is test or CI. Re-check logic carefully.
+ if (os.Getenv("GO_ENVIRONMENT") != "test" && os.Getenv("GO_ENV") != "test" && os.Getenv("ENVIRONMENT") != "test" && !core.HasEnv("CI")) {
panic("UnsafeUpdateCoingeckoMap can only be called in test environments")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // UnsafeUpdateCoingeckoMap updates the coingeckoIDLookup map with the provided entries. | |
| // This method is only meant to be used in test environments and will return an error if used in production. | |
| func UnsafeUpdateCoingeckoMap(entries map[string]string) error { | |
| // Only allow updates in test environments | |
| if !(os.Getenv("GO_ENVIRONMENT") != "test" || os.Getenv("GO_ENV") != "test" || os.Getenv("ENVIRONMENT") != "test" || core.HasEnv("CI")) { | |
| panic("UnsafeUpdateCoingeckoMap can only be called in test environments") | |
| } | |
| coingeckoMu.Lock() | |
| defer coingeckoMu.Unlock() | |
| // Update the map with new entries | |
| for token, coinGeckoID := range entries { | |
| coingeckoIDLookup[token] = coinGeckoID | |
| } | |
| return nil | |
| } | |
| // UnsafeUpdateCoingeckoMap updates the coingeckoIDLookup map with the provided entries. | |
| // This method is only meant to be used in test environments and will return an error if used in production. | |
| func UnsafeUpdateCoingeckoMap(entries map[string]string) error { | |
| // Only allow updates in test environments | |
| // Example: Ensure environment is test or CI. Re-check logic carefully. | |
| if (os.Getenv("GO_ENVIRONMENT") != "test" && os.Getenv("GO_ENV") != "test" && os.Getenv("ENVIRONMENT") != "test" && !core.HasEnv("CI")) { | |
| panic("UnsafeUpdateCoingeckoMap can only be called in test environments") | |
| } | |
| coingeckoMu.Lock() | |
| defer coingeckoMu.Unlock() | |
| // Update the map with new entries | |
| for token, coinGeckoID := range entries { | |
| coingeckoIDLookup[token] = coinGeckoID | |
| } | |
| return nil | |
| } |
| // simplify label for DirectUSD special identifier | ||
| if baseToken == "DirectUSD" { | ||
| baseToken = "USD" | ||
| } | ||
| if pricedToken == "DirectUSD" { | ||
| pricedToken = "USD" | ||
| } |
There was a problem hiding this comment.
Looks like this will need to be removed to be compatible with the rest of codebase using comparisons to "DirectUSD"?
Also, "DirectUSD" could be defined as a constant at this point
There was a problem hiding this comment.
hmmmm - i went with "DirectUSD" with the intention of making it crystal clear this is a flag for special behavior and has nothing to do with any crypto asset named "USD" or a variant of it (USDC etc)
But then showing "DirectUSD" ends up being confusing on downstream outputs & logs so I'm relabeling it to just "USD" here.
In hindsight this seems silly & adds to confusion more than it avoids it. I think i'll just call it "USD" throughout the process rather than "DirectUSD" & we'll get the idea.
There was a problem hiding this comment.
Let's use a single concise constant that no one will 100% be using to define the token name in the config. Maybe start with a special symbol like _USD or !USD or anything else that looks minimal enough to reflect the point?
There was a problem hiding this comment.
@parodime the "USD" naming should also work, but let's export a standalone constant for that (probably in fetcher.go?) for better readability.
| // This method is only meant to be used in test environments and will return an error if used in production. | ||
| func UnsafeUpdateCoingeckoMap(entries map[string]string) error { | ||
| // Only allow updates in test environments | ||
| if !(os.Getenv("GO_ENVIRONMENT") != "test" || os.Getenv("GO_ENV") != "test" || os.Getenv("ENVIRONMENT") != "test" || core.HasEnv("CI")) { |
There was a problem hiding this comment.
Inconsistent "is test" logic here with the above function. Also I believe this will not panic in prod as implemented.
There was a problem hiding this comment.
@trajan0x i think this is your update?
There was a problem hiding this comment.
Resolved most of my previous comments 💪
The only new one is this: #3532 (comment)
And then there are some merge conflicts, presumably from #3543 (cc @trajan0x).
I think this is in a good shape for the staging environment, but some cleaning up is needed prior to the master merge. Like the secondary price source probably deserves a separate PR, if we want to avoid keeping this PR open for way too long.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -29,6 +32,8 @@ type FeePricer interface { | |||
| GetGasPrice(ctx context.Context, chainID uint32) (*big.Int, error) | |||
| // GetTokenPrice returns the price of a token in USD. | |||
| GetTokenPrice(ctx context.Context, token string) (float64, error) | |||
| // GetPricePair calculates the price of a token pair from one chain to another. | |||
| GetPricePair(parentCtx context.Context, stepLabel string, baseTokenChain uint32, pricedTokenChain uint32, baseToken string, pricedToken string, baseValueWei big.Int) (_ *PricedValuePair, err error) | |||
There was a problem hiding this comment.
I would be very clear what all the terminology here means. My understanding from looking at the impl is that 'baseToken' would be ETH and 'pricedToken' would be USDC for example, which makes sense. Conventionally these would be called 'baseToken' and 'quoteToken' but I understand if you don't want to use quoteToken since we use 'quote' a lot in different contexts
There was a problem hiding this comment.
right -- i wanted to avoid "quote" because this function is used many times throughout the quote process (and elsewhere) and the amount being priced is not necessarily anything that will be quoted anywhere in a meaningful way
Similarly i wanted to avoid anything referring to in/out, from/to, orig/dest, start/end because that implies some type of bridge direction, which is irrespective of the pricing being performed within the function
I'll add some more to the func description to help clarify
| "go.opentelemetry.io/otel/attribute" | ||
| "go.opentelemetry.io/otel/trace" | ||
| ) | ||
|
|
||
| // exported constant used to apply special pricing behavior for USD as opposed to any token asset | ||
| // eg: If we are pricing an ETH value into US Dollars (not USDC) then we can supply USD_ as the "Price" asset | ||
| const USD_ = "USD" |
There was a problem hiding this comment.
Nit: if we moved this to a 'shared' package somewhere we could use it in places like here https://github.com/synapsecns/sanguine/pull/3532/files#diff-9ca0a09066e33602f302f0fc29e67fc6438ab782e1b9b2e2ee7cc0096088b990R689
I assume don't do this already due to circular dependency concerns
There was a problem hiding this comment.
not opposed to it but will just mark w/ TODO for now
| // check relay amount | ||
| // IE: what is the maximum amount of output tokens that we're willing & able to give in exchange for the amount of origin tokens supplied? |
There was a problem hiding this comment.
Hm, do we need to do the amount check for destination chain here as well?
There was a problem hiding this comment.
actually yes i think we should now -- adding
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit