-
Notifications
You must be signed in to change notification settings - Fork 35
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
v1.18.0 mainnet upgrade handler for net asset values and block height of pricing timestamp #1888
v1.18.0 mainnet upgrade handler for net asset values and block height of pricing timestamp #1888
Conversation
WalkthroughThis update introduces a feature to manage net asset values (NAVs) with block heights for markers in the Changes
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- app/navs_mainnet.go (1 hunks)
- app/upgrades.go (2 hunks)
- app/upgrades_test.go (2 hunks)
- x/marker/keeper/keeper.go (1 hunks)
Additional comments: 8
x/marker/keeper/keeper.go (1)
- 321-342: The implementation of
SetNetAssetValueWithBlockHeight
correctly updates the net asset value with a specific block height, validates the net asset value, emits an event, and stores the updated value. However, consider the following improvements for robustness and security:
- Ensure that the
marker
andnetAssetValue
parameters are validated at the beginning of the function to prevent processing invalid data.- Verify that all possible error paths are handled appropriately to ensure that the function behaves predictably in all scenarios.
Overall, the function aligns with the objectives of updating net asset values with specific block heights and enhancing transparency and accuracy in marker valuations.
app/upgrades.go (2)
- 316-334: The implementation of
addMarkerNavsWithHeight
correctly iterates over net asset values with heights and updates the corresponding markers. Consider the following improvements for enhanced robustness and clarity:
- Ensure comprehensive error handling to address potential issues during marker retrieval and net asset value updates.
- Enhance logging to provide more detailed information for debugging purposes, especially in error scenarios.
These improvements will help maintain the function's reliability and ease troubleshooting in the future.
- 154-154: The addition of
addMarkerNavsWithHeight
to thetourmaline
upgrade handler is appropriate and aligns with the upgrade's objectives of setting net asset values with heights for markers. Ensure that the function is called with the correct parameters and at the right point in the upgrade process to maintain the integrity and accuracy of the upgrade.app/upgrades_test.go (2)
- 461-462: The addition of logging for "Adding marker net asset values with heights." and its completion is a good practice for transparency during the upgrade process. It helps in debugging and provides clear checkpoints within the upgrade logs.
- 815-889: This test function
TestAddMarkerNavsWithHeight
effectively covers the scenario of adding marker net asset values with specific heights. However, it's crucial to ensure that the test cases cover a wide range of scenarios, including edge cases such as markers with zero supply or markers that already have NAVs set. Additionally, verifying the absence of NAVs for markers where the addition is expected to fail (e.g.,nosupplycoin1
) is a good practice to ensure the robustness of the upgrade handler.One improvement could be to add more comments within the test to explain the purpose of each block or scenario being tested, enhancing readability and maintainability for future developers.
CHANGELOG.md (1)
- 42-42: The addition of the upgrade handler for setting net asset values and updating block height for
pio-mainnet-1
is a significant feature. Ensure that the implementation aligns with the project's objectives for accuracy and transparency in marker valuations.app/navs_mainnet.go (2)
- 12-12: The TODO comment mentions removing functionality related to saffron handlers. It would be beneficial to provide more context or link to a tracking issue for better tracking and understanding of this future task.
- 12-12: The function
GetPioMainnet1DenomToNav
returns hardcoded net asset values. Consider fetching these values from a configuration file or an external service to improve maintainability and flexibility, especially if these values are subject to frequent updates or changes.
… of pricing timestamp (#1888) * add nav setting method to update block height * add new updates struct for navs, add recent navs * add upgrade nav setter * call upgrade nav adding funtion upgrade for v1.18.0 * add tests, add todo for removal * change marker names * fix test * add changelog entry * remove nhash from list * fix final test error
… of pricing timestamp (#1888) (#1890) * add nav setting method to update block height * add new updates struct for navs, add recent navs * add upgrade nav setter * call upgrade nav adding funtion upgrade for v1.18.0 * add tests, add todo for removal * change marker names * fix test * add changelog entry * remove nhash from list * fix final test error Co-authored-by: Carlton Hanna <nullpointer0x00@gmail.com>
Description
This PR updates marker Net Asset Values (NAV) on the blockchain using the latest data from the Figures Pricing API, ensuring values accurately reflect current market conditions. The API provides prices like $1.23, which we represent as an integer 1230 to maintain precision. Data is sourced from
https://figure.tech/service-pricing-engine/external/api/v1/pricing/marker/new?time=2024-03-21T00:00:00.000000Z
. Each update aligns the NAV with the blockchain block height according to thepricingTimestamp
, enhancing transparency and accuracy in marker valuations.closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passesSummary by CodeRabbit
New Features
Chores
Tests