Explorer Arbitrum fix [goreleaser] [SLT-263] #3188
Conversation
WalkthroughThe changes introduce additional token entries to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
services/explorer/static/tokenSymbolToCoinGeckoID.yaml (1)
5-6: Suggestion: Consider a more robust solution for unavailable pricesWhile changing the CoinGecko IDs to
'NO_PRICE'solves the immediate issue, it might be beneficial to implement a more robust solution for handling unavailable prices.Consider the following suggestions:
- Implement a fallback mechanism that attempts to fetch prices from alternative sources when CoinGecko fails.
- Create a separate configuration file for tokens with unavailable prices, allowing for easier management and updates.
- Implement a dynamic system that can detect and handle unavailable prices at runtime, reducing the need for manual updates to this file.
These approaches could provide more flexibility and resilience in handling similar issues in the future.
services/explorer/consumer/parser/tokendata/cache.go (1)
290-290: LGTM! Consider adding a comment for clarity.The addition of the WSOHM token to the
tokenDataMapis correct and aligns with the PR objectives. The chain ID (42161) corresponds to Arbitrum One, and the token details are accurately represented.For improved readability, consider adding a comment above this line to indicate that this entry is for the Arbitrum network:
+ // Arbitrum One "42161_0x30bD4e574a15994B35EF9C7a5bc29002F1224821": {"WSOHM", 18, "0x30bD4e574a15994B35EF9C7a5bc29002F1224821"},This would maintain consistency with any existing comments for other network-specific entries and make it easier for future developers to understand the purpose of this addition.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- services/explorer/consumer/parser/tokendata/cache.go (1 hunks)
- services/explorer/static/tokenIDToCoinGeckoID.yaml (1 hunks)
- services/explorer/static/tokenSymbolToCoinGeckoID.yaml (1 hunks)
🔇 Additional comments not posted (4)
services/explorer/static/tokenIDToCoinGeckoID.yaml (2)
Line range hint
1-43: Suggest comprehensive review of token mappings and clarification of 'NO_PRICE' vs 'NO_TOKEN'.While the changes for
wsOHMandJUMPaddress the immediate issue, I recommend:
- Conducting a comprehensive review of all token mappings to ensure they are up-to-date and accurate.
- Clarifying the distinction between
'NO_PRICE'and'NO_TOKEN'(e.g., veSOLAR is mapped to'NO_TOKEN').
- What's the difference in how the explorer handles these values?
- Should any existing
'NO_TOKEN'mappings be updated to'NO_PRICE'or vice versa?This will help maintain consistency and prevent similar issues with other tokens in the future.
To assist in this review, you can run:
#!/bin/bash # List all unique values in the YAML file yq e 'to_entries | .[] | .value' services/explorer/static/tokenIDToCoinGeckoID.yaml | sort | uniq # Count occurrences of each unique value yq e 'to_entries | .[] | .value' services/explorer/static/tokenIDToCoinGeckoID.yaml | sort | uniq -cThis will help identify any other non-standard values that might need attention.
5-6: Changes align with PR objectives, but verify impact on explorer functionality.The modifications to
wsOHMandJUMPtokens, setting their values to'NO_PRICE', align with the PR objectives of addressing the issue with the wsOHM token in the explorer. This change should prevent the explorer from stalling when attempting to fetch price data for these tokens.However, to ensure the robustness of this solution:
- Verify that the explorer code properly handles the
'NO_PRICE'value when encountered.- Consider adding a comment in the YAML file explaining why these tokens are set to
'NO_PRICE'.- Ensure that this change doesn't negatively impact any other parts of the system that might rely on price data for these tokens.
To verify the proper handling of
'NO_PRICE'in the explorer code:✅ Verification successful
Handling of
'NO_PRICE'VerifiedThe use of
'NO_PRICE'in the YAML files is properly managed within the explorer codebase:
- In
pricefetcher.go,'NO_PRICE'is checked to handle scenarios where price data is unavailable.- The constant
'NO_PRICE'is defined inbridgeparser.go, ensuring consistency across the codebase.No issues were found with the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for handling of 'NO_PRICE' in the explorer code rg --type python -i "NO_PRICE" services/explorerLength of output: 342
Script:
#!/bin/bash # Search for occurrences and handling of 'NO_PRICE' in the explorer codebase rg -i "NO_PRICE" services/explorerLength of output: 515
services/explorer/static/tokenSymbolToCoinGeckoID.yaml (2)
5-5: Approved: wsOHM price fetch preventionThe change for
wsohmto'NO_PRICE'aligns with the PR objectives. This will prevent the explorer from attempting to fetch the price for wsOHM, which is no longer available from CoinGecko, thus addressing the issue described in the PR summary.
6-6: Clarification needed: JUMP token price changeThe
jumptoken has also been changed to'NO_PRICE'. While this follows the same pattern as the wsOHM change, it wasn't mentioned in the PR objectives.Could you please clarify why this change was made for the JUMP token? Is it facing similar issues with CoinGecko price availability?
Deploying sanguine-fe with
|
| Latest commit: |
5ed9407
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a5560349.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-explorer-add-gohm.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3188 +/- ##
===================================================
- Coverage 41.54443% 36.38322% -5.16121%
===================================================
Files 460 438 -22
Lines 25770 25531 -239
Branches 357 82 -275
===================================================
- Hits 10706 9289 -1417
- Misses 14326 15501 +1175
- Partials 738 741 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Defi-Moses
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
services/explorer/static/tokenSymbolToTokenID.yaml (2)
5-5: Consider updating related documentation and configurationsThe change in the token ID for wsOHM may require updates in other parts of the system. Please ensure that:
- Any documentation referencing wsOHM is updated to reflect this change.
- Other configuration files or code that might be using the old token ID are updated.
- Any API integrations or external services that rely on this token ID are notified or updated accordingly.
This will help maintain consistency across the entire system and prevent potential issues in the future.
Line range hint
1-41: File structure and importanceThis YAML file serves as a crucial mapping between token symbols and their corresponding token IDs. It plays a vital role in the explorer's functionality, especially in token identification and data retrieval processes.
The structure is simple and effective, with each line representing a key-value pair of
token_symbol: 'TOKEN_ID'. This format allows for easy maintenance and quick lookups.Given the importance of this file in the system:
- Ensure that all mappings are kept up-to-date with the latest token information.
- Consider adding comments for any non-obvious mappings or special cases.
- Regularly review and validate the contents of this file to maintain accuracy.
services/explorer/consumer/parser/tokendata/cache.go (1)
290-291: LGTM! Consider adding a comment for clarity.The addition of wsOHM token entries for Arbitrum (chain ID 42161) and Avalanche (chain ID 43114) is consistent with the existing pattern and addresses the PR objectives. This change will allow the explorer to properly handle the wsOHM token on these chains.
Consider adding a comment above these new entries to explain why they were added, e.g.:
// Added wsOHM entries to handle explorer indexing issues after removal from CoinGecko APIThis would provide context for future developers working on this code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- services/explorer/consumer/parser/bridgeparser.go (1 hunks)
- services/explorer/consumer/parser/rfqparser.go (1 hunks)
- services/explorer/consumer/parser/tokendata/cache.go (1 hunks)
- services/explorer/static/tokenIDToCoinGeckoID.yaml (1 hunks)
- services/explorer/static/tokenSymbolToCoinGeckoID.yaml (1 hunks)
- services/explorer/static/tokenSymbolToTokenID.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/explorer/static/tokenIDToCoinGeckoID.yaml
- services/explorer/static/tokenSymbolToCoinGeckoID.yaml
🔇 Additional comments (5)
services/explorer/static/tokenSymbolToTokenID.yaml (1)
5-5: Verify the updated token ID for wsOHMThe token ID for 'wsohm' has been changed from 'wsOHM' to 'governance-ohm'. This change aligns with the PR objective to address issues related to the wsOHM token.
To ensure consistency across the codebase, please run the following script:
✅ Verification successful
Token ID for 'wsohm' successfully updated
All references to the old token ID 'wsOHM' have been updated to 'governance-ohm' across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old wsOHM token ID # Test: Search for 'wsOHM' token ID. Expect: No results outside of git history or comments. rg --type yaml --type python -i 'wsohm.*wsOHM' -g '!*.md' # Test: Verify the new token ID is used consistently. Expect: Consistent usage of 'governance-ohm'. rg --type yaml --type python -i 'wsohm.*governance-ohm'Length of output: 179
Script:
#!/bin/bash # Description: Check for any remaining references to the old wsOHM token ID # Test: Search for 'wsOHM' token ID. Expect: No results outside of git history or comments. rg -g '*.yaml' -g '*.py' -i 'wsohm.*wsOHM' -g '!*.md' # Test: Verify the new token ID is used consistently. Expect: Consistent usage of 'governance-ohm'. rg -g '*.yaml' -g '*.py' -i 'wsohm.*governance-ohm' -g '!*.md'Length of output: 355
services/explorer/consumer/parser/rfqparser.go (2)
173-173: LGTM: Appropriate log level adjustmentChanging the log level from
WarnftoInfofis a good improvement. This log message indicates a successful operation (finding the token price), which aligns better with theInfolog level rather thanWarn.
179-179: LGTM: Consistent log level adjustmentThis change mirrors the previous one, maintaining consistency in logging practices throughout the function. It's good to see uniform treatment of similar log messages.
services/explorer/consumer/parser/tokendata/cache.go (2)
290-291: Changes are isolated and do not require modifications elsewhere.The additions to
tokenDataMapare self-contained and do not impact the existing logic in the file. The new entries for wsOHM will be seamlessly integrated into the token data retrieval process for the Arbitrum and Avalanche chains.
290-291: Overall assessment: Changes are appropriate and address the PR objectives.The additions to
tokenDataMapfor wsOHM tokens on Arbitrum and Avalanche chains are minimal, focused, and consistent with the existing code structure. These changes effectively address the explorer indexing issues related to the removal of wsOHM from the CoinGecko API, as outlined in the PR objectives.
| logger.Infof("RFQ GetAmountUSD properly found the token price for coingecko token: %s", coinGeckoID) | ||
| rfqEvent.AmountUSD = *amountUSD | ||
| } | ||
| } else if rfqEvent.EventType == rfqTypes.BridgeRelayedEvent.Int() { | ||
| amountUSD := GetAmountUSD(rfqEvent.DestinationAmount, *rfqEvent.TokenDecimal, tokenPrice) | ||
| if amountUSD != nil { | ||
| logger.Warnf("RFQ GetAmountUSD properly found the token price for coingecko token: %s", coinGeckoID) | ||
| logger.Infof("RFQ GetAmountUSD properly found the token price for coingecko token: %s", coinGeckoID) | ||
| rfqEvent.AmountUSD = *amountUSD |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce log duplication
While the log level changes are good, there's an opportunity to reduce code duplication. Both log messages are identical and appear in similar contexts within the same function.
Consider refactoring the logging into a separate function or using a single log statement. For example:
func logTokenPriceFound(coinGeckoID string) {
logger.Infof("RFQ GetAmountUSD properly found the token price for coingecko token: %s", coinGeckoID)
}
// Then in the applyPriceData function:
if amountUSD != nil {
logTokenPriceFound(coinGeckoID)
rfqEvent.AmountUSD = *amountUSD
}This approach would make future modifications easier and reduce the risk of inconsistencies.
| logger.Warnf("BRIDGE could not get token price for coingeckotoken; assuming price of 1: %s txhash %s %d", coinGeckoID, bridgeEvent.TxHash, bridgeEvent.TimeStamp) | ||
| one := 1.0 | ||
| tokenPrice = &one |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Unused IsPriceAssumed Field Detected
The proposed addition of bridgeEvent.IsPriceAssumed = true introduces a field that is not utilized elsewhere in the codebase. Unused fields can lead to confusion and clutter the codebase.
Recommendations:
-
Implement Usage: If
IsPriceAssumedis intended to be used in downstream processes or for debugging purposes, ensure that these implementations are completed. -
Remove Unused Field: If there are no plans to use this flag, consider removing the addition to maintain code clarity and quality.
🔗 Analysis chain
Improved error handling for missing token price data
The change from returning an error to logging a warning and assuming a default price improves the robustness of the function. However, there are some considerations:
-
The assumed price of 1.0 might lead to inaccurate calculations. Consider adding a flag or metadata to indicate when an assumed price is used.
-
The warning message could be more informative. Include additional context such as chain ID and token address.
Consider enhancing the warning message and adding a flag for assumed prices:
- logger.Warnf("BRIDGE could not get token price for coingeckotoken; assuming price of 1: %s txhash %s %d", coinGeckoID, bridgeEvent.TxHash, bridgeEvent.TimeStamp)
+ logger.Warnf("BRIDGE could not get token price; assuming price of 1. CoinGeckoID: %s, ChainID: %d, TokenAddress: %s, TxHash: %s, Timestamp: %d",
+ coinGeckoID, chainID, iFace.GetToken().Hex(), bridgeEvent.TxHash, *bridgeEvent.TimeStamp)
one := 1.0
tokenPrice = &one
+ bridgeEvent.IsPriceAssumed = true // Add this field to the BridgeEvent structTo ensure this change doesn't introduce unexpected behavior, let's verify the usage of tokenPrice:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of tokenPrice variable
rg 'tokenPrice' services/explorer/consumer/parser/bridgeparser.go
Length of output: 846
Script:
#!/bin/bash
# Search for usages of IsPriceAssumed in the codebase
rg 'IsPriceAssumed' services/explorer/consumer/parser/bridgeparser.go
Length of output: 69
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
services/explorer/consumer/fetcher/swapfetcher.go (2)
58-62: Improved error handling and loggingThe consolidated error handling with additional context (contract type, token index, and contract address) significantly improves debugging capabilities. Using the same detailed message for both logging and error returning ensures consistency.
Consider using a constant for the error message format to avoid repetition:
const errMsgFormat = "could not get %s token address for index %d from contract %s: %w" if err != nil { contractAddress := s.swapAddress.Hex() logger.Errorf(errMsgFormat, contractType, tokenIndex, contractAddress, err) return nil, fmt.Errorf(errMsgFormat, contractType, tokenIndex, contractAddress, err) }This would make future message updates easier and reduce the risk of inconsistencies.
47-64: Overall improvement in error handling and code structureThe refactoring of the
GetTokenAddressmethod has significantly improved its error handling, logging, and overall code structure. These changes enhance maintainability and debuggability without altering the core functionality of retrieving the token address. The modifications align well with Go best practices for error handling and logging.Consider applying similar improvements to other methods in this file, such as
GetTokenMetaData, to maintain consistency in error handling and logging across the entireswapFetcherstruct.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- services/explorer/consumer/fetcher/swapfetcher.go (1 hunks)
- services/explorer/consumer/parser/bridgeparser.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/explorer/consumer/parser/bridgeparser.go
🔇 Additional comments (2)
services/explorer/consumer/fetcher/swapfetcher.go (2)
47-50: Improved variable declarationsThe addition of variable declarations at the beginning of the function enhances code readability and sets up a more consistent error handling approach. This is a good practice in Go.
52-56: Enhanced contract type handlingThe addition of the
contractTypevariable improves error reporting by providing context about which contract type is being used. The logic for determining and using the appropriate contract (metaSwap or swap) is clear and concise.
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Adds checks for wsOHM so that explorer doesnt brick when trying to index. The problems was that coingecko removed wsOHM from their api and thus the explorer stalled.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements