Improve metadata scope nav cli command documentation #2160
Conversation
WalkthroughThe pull request introduces comprehensive updates to the documentation and functionality of the Changes
Assessment against linked issues
Possibly related PRs
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
CodeRabbit Configuration File (
|
![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
Outside diff range and nitpick comments (1)
.changelog/unreleased/improvements/2134-metadata-nav-doc.md (1)
1-1: Changelog entry looks good, consider adding more context.The changelog entry accurately describes the improvement and correctly links to the related GitHub issue. This is in line with the PR objectives and addresses the documentation gap mentioned in issue #2134.
Consider adding a brief note about the importance of this documentation to provide more context for users. For example:
-* Address missing documentation on metadata/scope nav command [#2134](https://github.com/provenance-io/provenance/issues/2134). +* Address missing documentation on metadata/scope nav command, improving user understanding of NAV functionality [#2134](https://github.com/provenance-io/provenance/issues/2134).This addition highlights the impact of the documentation improvement on user experience.
|
Hmm... so the nav design for metadata appears to have removed volume which makes the price/precision ratio invalid. While it is true that there is only one scope and it would seem that there should be only a volume of one each time this actually isn't correct. In cases where the asset is priced in a denomination where there is insufficient precision to represent the value as an integer the volume must be incremented beyond one to find an expression that results in the correct ratio. Further this keeps the NAV function identical to the marker implementation. |
|
Lots of issues uncovered here with the implementation diverging from the marker version ... this is going to be a tricky one to fix correctly ... |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (12)
.changelog/unreleased/improvements/2030-explicit-navs.md (1)
1-1: Approved with suggestions for improvement.The changelog entry effectively communicates the core change. However, consider enhancing it with the following suggestions:
- Provide a brief explanation of the impact on users or developers.
- Mention any potential breaking changes or necessary migration steps.
- Add a short note on why this change was made (e.g., to improve data integrity, reduce unintended side effects, etc.).
Example:
* Only set a NAV record when explicitly provided, improving data integrity and preventing unintended record creation. This change may require updates to systems that relied on implicit NAV record creation. [#2030](https://github.com/provenance-io/provenance/issues/2030)..changelog/unreleased/client-breaking/2158-fix-metadata-nav-example.md (1)
1-4: Comprehensive fix for metadata nav CLI command, but consider additional documentation.This changelog entry effectively addresses two important issues:
- Correcting the module name in the example (fixing Example provided for Add NAV command in metadata is wrong #2058).
- Adding the previously missing volume parameter for accurate price ratio calculations.
The addition of the volume parameter with a default value of 1 is a good approach to maintain backwards compatibility. However, given the client-breaking nature of this change, consider the following recommendations:
- Provide more detailed examples in the CLI documentation showcasing how to use the new volume parameter.
- Update related documentation or code comments to reflect this change, ensuring consistency across the codebase.
- Consider adding a migration guide or upgrade notes for existing clients who may need to adapt their implementations.
To further improve the documentation and ease the transition for users:
- Add examples to the CLI documentation showing both cases: with and without specifying the volume parameter.
- Update any related code comments or function documentation to mention the volume parameter and its default behavior.
- Create a short migration guide explaining how existing clients should adapt to this change, if necessary.
app/scope_navs_updater_test.go (1)
Line range hint
1-114: Consider expanding test coverage for the updatedNewNetAssetValuefunction.The changes in this file consistently update all
NewNetAssetValuefunction calls to include a new parameter set to 1. While the existing test cases have been adapted to this change, there might be room for improvement in the test coverage.Consider the following enhancements to the test suite:
- Add test cases that use different values for the new parameter in
NewNetAssetValueto ensure correct behavior across various scenarios.- Include edge cases, such as zero or negative values for the new parameter, if applicable.
- If the new parameter affects the NAV calculation, add assertions to verify the correctness of the calculated values.
These additions would help ensure that the new NAV representation is thoroughly tested and behaves as expected under different conditions.
x/metadata/keeper/genesis.go (1)
Line range hint
1-180: Summary of changes and considerationsThe changes introduced in this file address an important issue related to net asset values by ensuring a minimum volume of 1. This aligns with the marker implementation and prevents invalid price/precision ratios. However, there are several important considerations:
Backwards Compatibility: The change may affect existing genesis states with volumes less than 1. A migration strategy or backwards compatibility mechanism should be considered.
Documentation: The rationale behind this change should be clearly documented, both in code comments and in any relevant specification documents.
Testing: Comprehensive testing should be performed to ensure this change doesn't introduce unexpected behavior in other parts of the system that may rely on the previous behavior.
Logging and Monitoring: Consider adding logging or event emission when volume adjustments occur to aid in system monitoring and debugging.
Configurability: Evaluate whether making the minimum volume configurable via module parameters would provide beneficial flexibility for future adjustments.
Please review these considerations and update the PR accordingly. It may be helpful to discuss these changes with the broader team to ensure alignment with the overall system architecture and upgrade strategy.
proto/provenance/metadata/v1/events.proto (1)
184-184: Approved: Addition of volume field addresses implementation concerns.The addition of the
volumefield to theEventSetNetAssetValuemessage is a positive change that addresses the concerns raised in the PR comments. This modification allows for more accurate representation of net asset values, especially in cases where the volume needs to exceed 1 to maintain precision.Consider adding a comment to explain the purpose and potential values of the
volumefield, especially noting cases where it might exceed 1. This would enhance the clarity of the proto file. For example:message EventSetNetAssetValue { string scope_id = 1; string price = 2; string source = 3; - string volume = 4; + // volume represents the quantity of the asset. It's a string to allow for precise representation, + // and may exceed 1 in cases where the asset is priced in a denomination lacking sufficient precision. + string volume = 4; }x/metadata/types/scope.go (2)
588-592: LGTM! Consider adding basic validation.The changes to
NewNetAssetValuefunction look good. The addition of thevolumeparameter addresses the concerns raised in the PR comments about maintaining consistency with the marker implementation and allowing for more accurate representation of values.Consider adding basic validation for the
volumeparameter to ensure it's not zero, as a zero volume might not make sense in the context of net asset value. You could modify the function like this:-func NewNetAssetValue(price sdk.Coin, volume uint64) NetAssetValue { +func NewNetAssetValue(price sdk.Coin, volume uint64) (NetAssetValue, error) { + if volume == 0 { + return NetAssetValue{}, errors.New("volume must be greater than zero") + } return NetAssetValue{ Price: price, Volume: volume, - } + }, nil }This change would require updating the callers of this function to handle the potential error.
Line range hint
594-596: Update Validate method to include Volume checkThe
Validatemethod ofNetAssetValuecurrently only validates thePricefield. With the recent addition of theVolumefield, it's important to update this method to ensure complete validation of theNetAssetValuestruct.Please update the
Validatemethod to include a check for theVolumefield. Here's a suggested implementation:func (mnav *NetAssetValue) Validate() error { - return mnav.Price.Validate() + if err := mnav.Price.Validate(); err != nil { + return err + } + if mnav.Volume == 0 { + return errors.New("volume must be greater than zero") + } + return nil }This change ensures that both
PriceandVolumeare validated, maintaining consistency with theNewNetAssetValuefunction and preventing potential issues with zero volume values.x/metadata/keeper/msg_server.go (1)
49-58: Approve changes with suggestions for improvementThe changes effectively prevent the creation of zero-value NAV entries, which is a good improvement. However, consider the following suggestions:
- The quantity is hardcoded to 1. Consider making this configurable or deriving it from the message if needed.
- The error handling could be more specific. Consider wrapping the error with more context about the NAV addition failure.
Additionally, could you please clarify how these changes address the concern raised in the PR comments about the removal of volume from the implementation? It seems this aspect isn't directly addressed in the current changes.
Consider applying these improvements:
if msg.UsdMills > 0 { usdMills := sdkmath.NewIntFromUint64(msg.UsdMills) - nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills), 1) + quantity := uint64(1) // Consider making this configurable or deriving from the message + nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills), quantity) err := k.AddSetNetAssetValues(ctx, msg.Scope.ScopeId, []types.NetAssetValue{nav}, types.ModuleName) if err != nil { - return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) + return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to add NAV for scope %s: %v", msg.Scope.ScopeId, err) } }x/marker/keeper/msg_server.go (4)
117-127: Approve changes with a minor suggestion.The addition of the conditional check for creating NAV entries is a good improvement. It prevents the creation of incorrect NAV entries when setting up markers and allows for explicit zero values to be set in subsequent calls.
Consider extracting the NAV creation logic into a separate method to improve readability and maintainability. This would also help ensure consistency with the
AddFinalizeActivateMarkerfunction. For example:+func (k msgServer) createNavEntryIfNeeded(ctx sdk.Context, ma *types.MarkerAccount, usdMills uint64, volume sdkmath.Int) error { + if usdMills > 0 { + usdMillsInt := sdkmath.NewIntFromUint64(usdMills) + nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMillsInt), volume) + return k.AddSetNetAssetValues(ctx, ma, []types.NetAssetValue{nav}, types.ModuleName) + } + return nil +} // In AddMarker function -if msg.UsdMills > 0 { - usdMills := sdkmath.NewIntFromUint64(msg.UsdMills) - nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills), msg.Volume) - err = k.AddSetNetAssetValues(ctx, ma, []types.NetAssetValue{nav}, types.ModuleName) - if err != nil { - return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) - } -} +if err := k.createNavEntryIfNeeded(ctx, ma, msg.UsdMills, msg.Volume); err != nil { + return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) +}This refactoring would make the code more modular and easier to maintain.
525-532: Approve changes with a consistency suggestion.The addition of the conditional check for creating NAV entries in the
AddFinalizeActivateMarkerfunction is consistent with the changes made in theAddMarkerfunction. This improvement prevents the creation of incorrect NAV entries when setting up markers.For consistency and improved maintainability, consider using the same refactoring suggestion provided for the
AddMarkerfunction. Extract the NAV creation logic into a separate method:+func (k msgServer) createNavEntryIfNeeded(ctx sdk.Context, ma *types.MarkerAccount, usdMills uint64, volume sdkmath.Int) error { + if usdMills > 0 { + usdMillsInt := sdkmath.NewIntFromUint64(usdMills) + nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMillsInt), volume) + return k.AddSetNetAssetValues(ctx, ma, []types.NetAssetValue{nav}, types.ModuleName) + } + return nil +} // In AddFinalizeActivateMarker function -if msg.UsdMills > 0 { - usdMills := sdkmath.NewIntFromUint64(msg.UsdMills) - err = k.AddSetNetAssetValues(ctx, ma, []types.NetAssetValue{types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMills), msg.Volume)}, types.ModuleName) - if err != nil { - return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) - } -} +if err := k.createNavEntryIfNeeded(ctx, ma, msg.UsdMills, msg.Volume); err != nil { + return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) +}This refactoring would ensure consistency between both functions and make the code more modular and easier to maintain.
117-127: Consistent changes with opportunity for improvement.The changes made to both
AddMarkerandAddFinalizeActivateMarkerfunctions are consistent in their approach to handling NAV entry creation. Both now include a conditional check to create NAV entries only whenmsg.UsdMills > 0.To further improve code quality and maintainability, consider extracting the common NAV creation logic into a separate method, as suggested in the previous comments. This would reduce code duplication and ensure that any future changes to this logic are applied consistently across both functions.
Here's a summary of the suggested refactoring:
- Create a new method
createNavEntryIfNeeded:func (k msgServer) createNavEntryIfNeeded(ctx sdk.Context, ma *types.MarkerAccount, usdMills uint64, volume sdkmath.Int) error { if usdMills > 0 { usdMillsInt := sdkmath.NewIntFromUint64(usdMills) nav := types.NewNetAssetValue(sdk.NewCoin(types.UsdDenom, usdMillsInt), volume) return k.AddSetNetAssetValues(ctx, ma, []types.NetAssetValue{nav}, types.ModuleName) } return nil }
- Update both
AddMarkerandAddFinalizeActivateMarkerto use this new method:if err := k.createNavEntryIfNeeded(ctx, ma, msg.UsdMills, msg.Volume); err != nil { return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) }This refactoring would improve code organization, reduce duplication, and make future maintenance easier.
Also applies to: 525-532
Line range hint
1-858: Summary of changes and recommendationsThe modifications to the
x/marker/keeper/msg_server.gofile focus on improving the handling of Net Asset Value (NAV) entries during marker creation. The changes are implemented consistently in both theAddMarkerandAddFinalizeActivateMarkerfunctions.Key improvements:
- Prevention of incorrect NAV entries when setting up markers.
- Allowing explicit zero values to be set in subsequent calls.
The changes are logical and address the identified issues effectively.
To further enhance code quality and maintainability, consider the following recommendations:
- Extract the common NAV creation logic into a separate method, as detailed in previous comments.
- Apply this refactoring to both
AddMarkerandAddFinalizeActivateMarkerfunctions.These refactoring steps will reduce code duplication, improve readability, and ensure consistency in future modifications related to NAV entry creation.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
x/metadata/types/events.pb.gois excluded by!**/*.pb.gox/metadata/types/scope.pb.gois excluded by!**/*.pb.go
Files selected for processing (14)
- .changelog/unreleased/client-breaking/2158-fix-metadata-nav-example.md (1 hunks)
- .changelog/unreleased/improvements/2030-explicit-navs.md (1 hunks)
- app/scope_navs_updater.go (1 hunks)
- app/scope_navs_updater_test.go (2 hunks)
- proto/provenance/metadata/v1/events.proto (1 hunks)
- proto/provenance/metadata/v1/scope.proto (1 hunks)
- x/marker/keeper/marker.go (0 hunks)
- x/marker/keeper/msg_server.go (2 hunks)
- x/marker/keeper/msg_server_test.go (0 hunks)
- x/metadata/client/cli/cli_test.go (2 hunks)
- x/metadata/client/cli/tx.go (3 hunks)
- x/metadata/keeper/genesis.go (1 hunks)
- x/metadata/keeper/msg_server.go (1 hunks)
- x/metadata/types/scope.go (1 hunks)
Files not reviewed due to no reviewable changes (2)
- x/marker/keeper/marker.go
- x/marker/keeper/msg_server_test.go
Additional context used
Learnings (1)
x/metadata/client/cli/tx.go (1)
Learnt from: iramiller PR: provenance-io/provenance#2160 File: x/metadata/client/cli/tx.go:1196-1212 Timestamp: 2024-09-23T22:46:26.850Z Learning: When checking for indentation issues in multi-line strings assigned to the `Long` field in Go code, verify that the source lines are actually indented before flagging them.
Additional comments not posted (12)
app/scope_navs_updater.go (1)
75-75:⚠️ Potential issueConsider a more flexible approach for handling volume in NetAssetValue
The change from
metadatatypes.NewNetAssetValue(price)tometadatatypes.NewNetAssetValue(price, 1)appears to be an attempt to address the volume issue mentioned in the PR objectives. However, this fixed value approach might not fully resolve the concerns raised about the NAV design and the need for variable volume.
Implications:
- Using a fixed volume of 1 might not be sufficient for cases where the asset is priced in a denomination lacking sufficient precision, as mentioned in the PR comments.
- This could potentially impact the price/precision ratio, which was a concern raised in the PR objectives.
Concerns:
- The current implementation might not align with the marker implementation, as mentioned in the PR comments.
- It may not provide the flexibility needed to accurately represent values in all cases.
Suggestions:
- Consider implementing a dynamic approach for determining the volume based on the precision requirements of the asset and its pricing denomination.
- You might want to add a function that calculates the appropriate volume based on the price and precision requirements, rather than using a fixed value.
- Alternatively, consider passing the volume as a parameter to the
ReadScopeNAVsfunction, allowing for more flexibility in how it's determined.To better understand the implications of this change, let's examine how
NewNetAssetValueis used elsewhere in the codebase:This will help us determine if this change is consistent with other usages and if similar changes are needed elsewhere.
app/scope_navs_updater_test.go (1)
26-26: Verify the purpose and impact of the new parameter inNewNetAssetValue.The addition of a new parameter (set to 1) in the
NewNetAssetValueconstructor suggests a change in the NAV calculation or representation. This aligns with the PR objectives mentioning NAV design changes.Could you please clarify:
- The purpose of this new parameter?
- How does it affect the NAV calculation?
- Are there any scenarios where this value would be different from 1?
To ensure consistency across the codebase, let's check for other occurrences of
NewNetAssetValue:This will help us verify if the change has been applied consistently and identify any potential areas that might need updating.
Verification successful
Consistent usage of
NewNetAssetValueconfirmed across the codebase.The addition of the new
volumeparameter in theNewNetAssetValueconstructor is uniformly applied, ensuring that NAV calculations and representations are handled consistently. Differentvolumevalues are appropriately used based on the context and requirements of each test case.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of NewNetAssetValue rg --type go 'NewNetAssetValue\(' -A 2Length of output: 12376
x/metadata/keeper/genesis.go (1)
70-75: Ensure volume adjustment aligns with overall system designThe introduction of a minimum volume of 1 is a significant change that warrants careful consideration:
While this change aligns with the marker implementation and prevents invalid price/precision ratios, it's important to ensure this adjustment is consistent with the overall system design and doesn't introduce unexpected side effects.
Consider the backwards compatibility implications of this change. Existing genesis states with volumes less than 1 will now be interpreted differently, which could lead to inconsistencies if not properly managed during upgrades.
It might be beneficial to add some form of logging or event emission when this adjustment occurs, to provide visibility into cases where the volume is being automatically adjusted.
Consider the following suggestions:
Add a comment explaining the rationale behind this volume adjustment to improve code maintainability.
Implement a migration strategy for existing genesis states with volumes less than 1, if applicable.
Add logging when the volume is adjusted. For example:
if volume < 1 { + k.Logger(ctx).Info("Adjusting NAV volume to minimum of 1", "original_volume", volume, "address", mNavs.Address) volume = 1 }
- Consider making this minimum volume configurable via module parameters, allowing for future adjustments without code changes.
To ensure this change doesn't introduce unexpected behavior, please run the following verification:
This will help identify any other parts of the codebase that might be affected by this change.
Verification successful
Volume Adjustment Verification Successful
The analysis confirms that there are no existing
NetAssetValueinstances with a volume less than 1 in the codebase. All current usages set the volume to 1, ensuring consistent and valid price/precision ratios throughout the system.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing NAVs with volume less than 1 in the codebase # Test: Search for NAV creation or updates with volume less than 1 rg --type go 'NewNetAssetValue.*,\s*([0-9]*\.)?[0-9]+\s*[,)]' -g '!*test*'Length of output: 351
proto/provenance/metadata/v1/scope.proto (1)
232-235: Approve the addition of thevolumefield with suggestions for documentation and implementation.The addition of the
uint64 volumefield to theNetAssetValuemessage is a good solution to address the precision issues mentioned in the PR comments. It allows for more accurate representation of asset values when the price denomination's precision is insufficient.Consider the following suggestions to ensure smooth integration of this change:
- Update any existing code that interacts with the
NetAssetValuemessage to handle the newvolumefield appropriately.- Enhance the documentation in the
03_messages.mdfile to explain when and how to use thevolumefield, especially in cases where it needs to be greater than one.- Add examples in the documentation showcasing scenarios where the
volumefield is crucial for accurate price representation.- Update any relevant events in the
06_events.mdfile to include information about thevolumefield when NAV updates occur.- Consider adding validation logic in the implementation to ensure that the
volumefield is used correctly (e.g., defaulting to 1 when not specified, validating that it's only > 1 when necessary).To ensure consistency across the codebase, let's check for any other occurrences of
NetAssetValuethat might need updating:This will help identify any other places where the
NetAssetValuemessage is used or referenced, ensuring we don't miss any necessary updates.Verification successful
The review comment is verified; no code was removed or replaced in
proto/provenance/metadata/v1/scope.protoat lines 232-235. The only change involves adding thevolumefield to theNetAssetValuemessage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of NetAssetValue in the codebase rg --type proto --type go "NetAssetValue"Length of output: 71
Script:
#!/bin/bash # Search for other occurrences of NetAssetValue in .proto and .go files rg "NetAssetValue" --glob "*.proto" --glob "*.go" -nLength of output: 129224
x/metadata/types/scope.go (1)
Line range hint
1-596: Verify NetAssetValue struct definitionThe changes to
NewNetAssetValuefunction and the suggested update to theValidatemethod address the concerns raised in the PR comments. However, to ensure complete consistency, we need to verify that theNetAssetValuestruct definition includes theVolumefield.Please run the following script to check the
NetAssetValuestruct definition:If the
NetAssetValuestruct definition doesn't include theVolumefield, please update it accordingly.Verification successful
NetAssetValue struct includes
VolumefieldVerified that the
NetAssetValuestruct definition includes theVolumefield in:
x/metadata/types/scope.pb.go(line 764)x/marker/types/marker.pb.go(line 237)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify NetAssetValue struct definition includes Volume field # Test: Search for NetAssetValue struct definition ast-grep --lang go --pattern $'type NetAssetValue struct { $$$ Volume uint64 $$$ }' # If the above command doesn't return any results, the struct definition might need to be updated.Length of output: 1945
x/metadata/client/cli/tx.go (1)
1195-1234: LGTM! Improved command documentation and usage.The changes to the
GetCmdAddNetAssetValuesfunction significantly improve its usability and clarity. The expanded description and examples provide clear guidance on how to use the command in various scenarios. The updated usage string and aliases make the command more intuitive to use.x/metadata/client/cli/cli_test.go (6)
3863-3864: Update test case for multiple net asset valuesThe test case for multiple net asset values has been updated to use a semicolon (
;) as a separator instead of a comma (,). This change reflects an update in the expected format for specifying multiple net asset values.This change aligns with the new format for specifying multiple net asset values. It's a good practice to update tests to reflect changes in the underlying functionality.
3927-3932: Update test case for invalid volumeThe test case for an invalid volume has been modified. Previously, it was testing a comma-separated format. Now, it's testing the new format where volume is specified after a comma.
This change correctly reflects the new format for specifying net asset values. It's important to test that the old format is now considered invalid.
3935-3938: Update test case for single nav with volumeThe test case for parsing a single net asset value has been updated to include a volume specification.
This change correctly tests the new format where volume is specified after the price. It ensures that the parser can handle the new format correctly.
3940-3943: Update test case for single nav without volumeA new test case has been added to test parsing a single net asset value without an explicit volume.
This is a good addition. It tests the default behavior when no volume is specified, expecting a default volume of 1.
3947-3949: Update test case for multiple nav valuesThe test case for parsing multiple net asset values has been updated to use the new format with semicolons separating different values and commas separating price and volume.
This change correctly tests the new format for specifying multiple net asset values. It ensures that the parser can handle multiple values with different volumes correctly.
Line range hint
3863-3949: Comprehensive update to net asset value parsing and testingThe changes in this file reflect a significant update to how net asset values are specified and parsed. The new format uses semicolons to separate multiple values and commas to separate price and volume within each value. The test cases have been thoroughly updated to cover various scenarios with the new format, including:
- Multiple net asset values
- Single values with and without explicit volume
- Invalid formats
These changes appear to be well-thought-out and comprehensive. They ensure that the new format for net asset values is correctly implemented and tested. The updates to the test cases will help maintain the integrity of the net asset value functionality as the codebase evolves.
special handling of unset volume in nav to be one but allow zero for backwards compatibility
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
x/metadata/types/events.go (1)
Line range hint
1-289: Summary: NAV functionality improved with volume additionThe changes to this file effectively address the PR objectives by incorporating volume into the
NewEventSetNetAssetValuefunction. This addition aligns with the concerns raised about the price/precision ratio in the metadata NAV implementation.Key points:
- The
strconvpackage is correctly imported to handle the new volume conversion.- The
NewEventSetNetAssetValuefunction signature is updated to include thevolumeparameter.- The volume is properly converted to a string representation in the event structure.
These changes should improve the accuracy and flexibility of the NAV functionality. However, it's crucial to ensure that all parts of the codebase that interact with this function or process the resulting event are updated accordingly.
Consider the following to further improve the implementation:
- Update any relevant documentation to reflect the new volume parameter.
- Ensure that any code that processes
EventSetNetAssetValueevents is updated to handle the new volume field.- Consider adding validation for the volume parameter to ensure it's within expected ranges.
- If not already present, consider adding unit tests to verify the behavior of this function with various input combinations.
x/metadata/keeper/scope_test.go (2)
2553-2553: Consider using a variable for the volume parameterThe volume parameter in the
NewEventSetNetAssetValuefunction call is currently hardcoded as 1. It would be more flexible and allow for testing different scenarios if this value was stored in a variable or derived fromtc.netAssetValue.- event := types.NewEventSetNetAssetValue(scopeID, tc.netAssetValue.Price, 1, "test") + volume := uint32(1) // or derive this from tc.netAssetValue if applicable + event := types.NewEventSetNetAssetValue(scopeID, tc.netAssetValue.Price, volume, "test")This change would make the test more adaptable to future modifications in the
NetAssetValuestruct or event creation logic.
2591-2591: LGTM! Consider using a constant for consistencyThe addition of the
Volumefield to theNetAssetValuestruct is correct and consistent with the changes in theNetAssetValuedefinition. However, to maintain consistency across tests and make future updates easier, consider defining a constant or variable for the volume value.+ const testVolume = uint32(1) netAssetValue: types.NetAssetValue{ Price: sdk.Coin{ Denom: "usd", Amount: sdkmath.NewInt(1000), }, - Volume: 1, + Volume: testVolume, },This change would make it easier to update the volume value across all tests if needed in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- x/metadata/client/cli/tx.go (3 hunks)
- x/metadata/keeper/scope.go (2 hunks)
- x/metadata/keeper/scope_test.go (3 hunks)
- x/metadata/types/events.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/metadata/client/cli/tx.go
Additional comments not posted (4)
x/metadata/types/events.go (1)
4-5: LGTM: Import statement added correctly.The
strconvpackage import is appropriately added. It's necessary for the new functionality in theNewEventSetNetAssetValuefunction.x/metadata/keeper/scope.go (2)
704-708: Ensure volume is at least 1 for net asset values.This change adds a check to ensure that the
Volumefield of aNetAssetValueis always at least 1. This is a good practice to prevent division by zero errors and maintain consistency with previous behavior.
709-711: Include Volume in setNetAssetValueEvent.The
NewEventSetNetAssetValuefunction call now includes thenetAssetValue.Volumeparameter. This change ensures that the Volume information is properly recorded in the emitted event, maintaining consistency between the state and events.x/metadata/keeper/scope_test.go (1)
2609-2609: LGTM! Comprehensive assertionThe update to the assertion is correct and comprehensive. It now compares the entire
tc.netAssetValuestruct, including the newly addedVolumefield. This ensures that all aspects of theNetAssetValue, including the volume, are correctly set and retrieved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/metadata/spec/06_events.md (2)
12-12: Adjust indentation in the table of contents.The indentation for the new
EventSetNetAssetValueentry in the table of contents is inconsistent with other entries. This is causing a Markdownlint warning.Please adjust the indentation to match other entries:
- - [EventSetNetAssetValue](#eventsetnetassetvalue) + - [EventSetNetAssetValue](#eventsetnetassetvalue)🧰 Tools
Markdownlint
12-12: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
85-87: Consider adding a comma for improved readability.There's a potential missing comma in the description that might improve readability.
Consider adding a comma after "one" in the following sentence:
-Note: Generally the volume should be set to one however if the units of the +Note: Generally the volume should be set to one, however if the units of the price token are not sufficiently precise the volume can be used to define a ratio of integers to represent the decimal.🧰 Tools
LanguageTool
[uncategorized] ~85-~85: Possible missing comma found.
Context: ...: Generally the volume should be set to one however if the units of the price token...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/metadata/spec/06_events.md (2 hunks)
🧰 Additional context used
Markdownlint
x/metadata/spec/06_events.md
12-12: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
LanguageTool
x/metadata/spec/06_events.md
[uncategorized] ~85-~85: Possible missing comma found.
Context: ...: Generally the volume should be set to one however if the units of the price token...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments not posted (2)
x/metadata/spec/06_events.md (2)
79-97: LGTM: New event documentation is clear and consistent.The documentation for
EventSetNetAssetValueis well-structured and provides clear information about the event's purpose, type, and attributes. It follows the same format as other event descriptions in the file.🧰 Tools
LanguageTool
[uncategorized] ~85-~85: Possible missing comma found.
Context: ...: Generally the volume should be set to one however if the units of the price token...(AI_HYDRA_LEO_MISSING_COMMA)
Line range hint
1-97: Overall: New event documentation is well-integrated.The addition of the
EventSetNetAssetValuedocumentation maintains the structure and consistency of the file. The new event is properly placed within the "Scope" section and follows the established format for event descriptions.🧰 Tools
LanguageTool
[uncategorized] ~85-~85: Possible missing comma found.
Context: ...: Generally the volume should be set to one however if the units of the price token...(AI_HYDRA_LEO_MISSING_COMMA)
* [2137]: Remove the MetadataKeeperI interface since it wasn't being used anywhere and anything trying to use the metadata keeper should define their own interface. * [2137]: Add the bank keeper to the metadata keeper. * [2137]: Add a Denom method to the MetadataAddress type. * [2137]: Add BurnCoins to the BankKeeper interface since that'll be needed when a scope is deleted. * [2137]: Remove all of the value-owner -> index stuff with a couple TODOs for re-implementation. * [2137]: Create a struct that associates an acc address with a metadata address. * [2137]: Fix the import name of bank types in expected keepers. * [2137]: Create GetScopeValueOwner and GetScopeValueOwners keeper methods. * [2137]: Expand the Denom test to include all the different types. * [2137]: Create a Coin method on the MetadataAddress type. * [2137]: Create ValidateAllAreScopes and Coins methods on the AccMDLinks type. * [2137]: Remove InputOutputCoins from the interface and add GetSupply. * [2137]: Add the module address to the metadata keeper so it can be reused when needed. * [2137]: Rip out some more stuff from the UpdateValueOwners and MigrateValueOwner endpoints to make it easier to change the stuff called in there. * [2137]: Create a method on the AccMDLinks type that will return a new slice with nils removed (if it's got nils in it to begin with). * [2137]: Refactor SetScopeValueOwners to use the bank module for the stuff. * [2137]: Remove the GetAccAddrs and GetMetadataAddrs methods from AccMDLinks since I couldn't actually use them. * [2137]: Put the MetadataAddress Prefix method back to the way it was because the new way was sometimes returning an error when it shouldn't. * [2137]: Update IterateScopes to populate the value owner field. Create GetScopeWithValueOwner that gets the scope with the value owner populated. Update the comment on GetScope to indicate that it no longer has the value owner field populated. * [2137]: In SetScope, clear out the value owner so that it'll always be empty now. * [2137]: Create a PopulateScopeValueOwner method and use that in IterateScopes and GetScopeWithValueOwner. * [2137]: Update the queries that get scopes to get them with the value owner populated. * [2137]: In ValidateWriteScope, get the scope with the value owner since it's used in there. * [2137]: Refactor ValidateAllAreScopes into ValidateForScopes and have it stop at the first error, check for nil entries, and also make sure the AccAddr isn't empty. Delete the WithNilsRemoved method. Add TODO to tweak MetadataAddress.String() to not panic when the address is invalid. * [2137]: Create ValidateIsScopeAddress since I was using that same code in a few places. * [2137]: Tweak an ordering of checks and include the offending string in an error in the UpdateValueOwnersCmd. * [2137]: Create SetScopeValueOwner as the only place to mint/burn stuff (and only works on a single scope), and refactor SetScopeValueOwners to require that all of the coins already exist. * [2137]: Remove GetSupply and SpendableCoins from the BankKeeper interface and in the MockBankKeeper, impelement BlockedAddr, MintCoins BurnCoins and SendCoins. * [2137]: In SetScopeValueOwner, do the DenomOwner lookup after checking the newValueOwner to prevent that lookup when the value owner is bad or blocked and include the burn error if there is one. * [2137]: Unit tests on SetScopeValueOwner. * [2137]: Add a comment about the error string (that's still todo, but will eventually be done) in MetadataAddress.String(). * [2137]: Create a function to convert a denom back into the metadata address. * [2137]: Re-implement the ValueOwnership query. * [2137]: Add the metedate module to the list of module accounts and give it mint and burn permission. * [2137]: Add a mint coins restriction to the metadata module that ensures it can only mint specific stuff. * [2137]: Update SetScope and RemoveScope to set/update/delete the value owner entry too. * [2137]: In the UpdateValueOwnersCmd, make it return the error when it's not a scope (insted of just making the error and moving on). * [2137]: Add cosmossdk.io/collections to the allowed imports list. * [2137]: Fix import ordering in expected keepers. * [2137]: Pay attention to the new new error return value from SetScope and SetScopeValueOwners. * [2137]: Update the pagination in ValueOwnership to use CollectionPaginate and include the denom prefix in the iterator prefix. * [2137]: Create the GetAccAddrs method on AccMDLinks (again) since I now need to use it. * [2137]: Update the marker context helpers to allow providing more than one admin to check permissions for. * [2137]: Update the ValidateUpdateValueOwners method and re-implement the UpdateValueOwners and MigrateValueOwner endpoints. * [2137]: Remove MsgUpdateValueOwnersRequest and MsgMigrateValueOwnerRequest from the message types that a MsgWriteScopeRequest authz grant works for. I.e. an authz grant for MsgWriteScopeRequest no longer confers usage of those value owner endpoints. * [2137]: Fix a couple uses of %w in non-error format strings. * [2137]: Remove a struct initilization that I originally used to have all the fields on-screen, but that served no functional purpose and should have been removed earlier. * [2137]: Redo unit tests on ValidateUpdateValueOwners and tweak it to just look for the required signers in the list instead of making a map for it. * [2137]: Add a todo to the bank keeper wrapper thingy to make it easier to mock the balances stuff. * [2137]: Create ValidateScopeValueOwnerSigned as a replacement to ValidateScopeValueOwnerUpdate and use it in ValidateWriteScope and ValidateDeleteScope. Mark ValidateScopeValueOwnerUpdate and some stuff only needed by it for removal. Have ValidateWriteScope look up the existig scope instead of haivng it be provided. Require there to be a value owner for new scopes (but might go back on that). * [2137]: Remove the DefaultParamspace variable since it's unused and fix the comment on NetAssetValueKeyPrefix. * [2137]: Create VerifyMetadataAddressHasType as a generic way to both validate a MetadataAddress and ensure it's a specific type. Switch ValidateIsScopeAddress to use this and create ValidateIsScopeSpecificationAddress that uses it. * [2137]: Update Scope.ValidateBasic to use the new ValidateIsScopeAddress and ValidateIsScopeSpecificationAddress. * [2137]: Split the scope writing portion of SetScope out into writeScopeToState so that it's easier to make unit tests that have a scope with a value owner in state. * [2137]: In MetadataAddress.String(), if it's not valid, have it output the %#v format instead of panicking. * [2137]: Add the transfer agents to the context in the endpoints that might try to update a value owner. * [2137]: Get rid of the AccMDLinks.Coins method since it wasn't used anymore. Clean up several comments and add some missing ones. * [2137]: Fix a few unit tests. TestSetScopeValueOwner started failling when I changed some error messages. TestValidateUpdateValueOwners needed some test cases on the returned addresses. TestWriteScopeSmartContractValueOwnerAuthz needed to have the scopes written so they could be looked up. * [2137]: Update RemoveScope to return an error instead of panicking. Fix the UpdateValueOwners to actually provide the links to SetScopeValueOwners. Fix a few spots that where trying to %q an AccAddress which makes it format as hex for some dumb reason. * [2137]: Delete the GetMarkerAndCheckAuthority, validateScopeValueOwnerChangeToProposed, validateScopeValueOwnerChangeFromExisting, and ValidateScopeValueOwnerUpdate methods since the latter isn't used anymore, and the others were only used by that flow. Create IsMarkerAddr and use it to not require markers to sign for value owner chagnes since the send restiction will check for permissions among the signers. * [2137]: Get rid of the GetBalancesCollection method on MDBankKeeper, and replace it with GetScopesForValueOwner. * [2137]: Unit tests on the DenomOwner MDBankKeeper method. * [2137]: Finish unit tests on the MDBankKeeper. * [2137]: Remove the mintCoinsRestriction since we're only minting in one place and its doing any needed checks. * [2137]: Replace the ValidateScopeValueOwnerSigned keeper method with ValidateScopeValueOwnersSigners. The new one has most of the content of ValidateUpdateValueOwners. Make all of WriteScope, DeleteScope, UpdateValueOwners, and MigrateValueOwner all use this same verification for value owner changes. Only provide the validatd signers as transfer agents. * [2137]: Take out the part where a value owner is required to write a scope and change the TODO on it to decide if we want to require it. * [2137]: Expand marker unit tests to account for the allowance of multiple transfer agents. * [2137]: Update marker flowcharts to reflect the possibility of multiple admins. * [2137]: In TestValidateDeleteScope, make it run all the tests when one panics. * [2137]: Unit tests on IsMarkerAddr. * [2137]: Create the IsMarkerAccount method on the marker keeper that does the check without having to get the account from auth. * [2137]: Get rid of the metadata module's version of IsMarkerAddr and switch to the marker module's version which is more efficient. * [2137]: Have ValidateScopeValueOwnersSigners return the transfer agents regardless of whether any of the existing accounts are marker accounts. It was either that or have it check the proposed too to see see if they might be needed. * [2137]: Unit tests on ValidateScopeValueOwnersSigners. * [2137]: Redo the tests on ValidateUpdateValueOwners since a bulk of that logic is now being tested on ValidateScopeValueOwnersSigners. * [2137]: Identify all of the keeper tests that fail and add TODOs to them. * [2137]: Identify all of the cli tests that fail and add TODOs to them. * [2137]: Move PartyDetails, UsedSignersMap, and the AuthzCache stuff out of the keeper and into types since they might be needed by people trying to use the metadata keeper. Also create the provutils package for some generic stuff that'd probably be handy in other places. * [2137]: Unit tests on Ternary and Pluralize and the rest of the signer utils that didn't have any yet. * [2137]: Update ValidateScopeValueOwnersSigners: If there's no existing owners, still process the signers and return them appropriately. * [2137]: In ValidateWriteScope, make sure there's a spec id. Update TestValidateWriteScope to fix all the stuff that has broken and add some new cases. * [2137]: Fix TestWriteScopeSmartContractValueOwnerAuthz. * [2137]: Overhaul and fix TestSetScopeValueOwners. * [2137]: Fix TestValidateDeleteScope and add a couple cases. * [2137]: Fix TestAddAndDeleteScopeDataAccess and TestAddAndDeleteScopeOwners. * [2137]: Redo the unit tests on the Scope query since they were failing and could use some added cases and stuff. * [2137]: Unit tests on the ScopesAll query. * [2137]: Remove the keeper's needsUpdate function since it's not needed anymore. Heh. * [2137]: Delete a couple TODO comments that are TODONE. * [2137]: Initial work on WriteScope tests. * [2137]: When reading a scope from state, clear out the ValueOwnerAddress field. * [2137]: Finish the unit tests on WriteScope. * [2137]: Create a Coins() method on the MetadataAddress struct that just wraps Coin(). * [2137]: Create the EventsBuilder in an attempt to make it easier to create the list of expected events for testing. * [2137]: Create a non-method way to write all the stuff in a dataSetup. * [2137]: Unit tests on the DeleteScope endpoint. Switch the WriteScope tests to use the new EventsBuilder. * [2137]: Fix TestScopesPagination. * [2137]: Re-enable TestScopeTxCommands. I must have fixed whatever had broken it earlier. * [2137]: Add a couple test cases to TestUpdateValueOwners, one of which fails because we're not requiring a signature for a scope that already has the desired value owner. * [2137]: Re-enable TestUpdateMigrateValueOwnersCmds and fix one of the failures. Need a code change to fix the others. * [2137]: Delete an empty line from the end of the TestDeleteOSLocator test runner. * [2137]: Create some helpers for writing stuff to the test logs. * [2137]: Create a GetMDAddrsForAccAddr method on the AccMDAddrs type. * [2137]: Add t.Helper() to the testlog stuff so that the log messages indicate the calling line instead of something inside logs.go. * [2137]: Get rid of the namedValue stuff in the metadata module and switch to using the testlog helpers. * [2137]: Switch GetMDAddrsForAccAddr to take in a string since that's what I've got where I want to use it. * [2137]: In ValidateUpdateValueOwners, return an error if any of the scopes already have the proposed value owner. * [2137]: Fix a test case in TestUpdateValueOwners that still had a TODO expected error. Get rid of the AssertErrorValue function defined in metadata/keeper/signers_test.go and use the one in our assertions package instead. * [2137]: Fix TestGetOwnershipCmd. It's the last of the broken unit tests. * [2137]: Fix the import section of keeper/signers_utils.go. * [2137]: In Scope.ValidateBasic, don't allow the spec id to be empty. * [2137]: Bump the metadata consensus version to 4 and create an empty migration to go from 3 to 4. * [2137]: Add TODO to update the spec docs. * [2137]: Create a Pair struct. Ugh. * [2137]: Write the metadata 3 to 4 migration. * [2137]: Remove the ValueOwnerScopeCacheKeyPrefix since it's now only need for the migration, which already has it. * [2137]: Delete the GetMaccPerms function since it's not used. * [2137]: Add the viridian upgrade. * [2137]: Create V3CreateScope to help facilitate testing, and switch the existing migration tests to use that. * [2137]: Fix import ordering in app/upgrades.go. * [2137]: Change the name of the panic-checking func in TestValidateWriteScope since it's the same as another function now. * [2137]: Write an upgrades test that creates 300,005 scopes and migrates them and checks things after. * [2137]: Bypass the marker send restriction in the metadata migration. * [2137]: Update the comment on the value_owner_address field in the proto. * [2137]: Update the line numbers in the spec docs links to scope.proto and also update the copy of the Scope message in there due to the new value owner comment. * [2137]: Update the spec docs to reflect the new value owner storage. Also fix a few typos in there. * [2137]: Add some changelog entries. * [2137]: Add a changelog entry for the viridian upgrade. * [2137]: Create a GetNetAssetValue method in the metadata module so that the exchange module can use it. * [2137]: In the exchange module, record scope navs when applicable. * [2137]: Use the correct formatting verb when logging the error about a metadata denom string not being valid. * [2137]: Update unit tests on the exchange keeper's GetNav method. * [2137]: Add some exchange test stuff involving scopes. * [2137]: Update the comments related to #2160. * [2137]: Tweak the migration to remove a panic that would happen if any scope somehow has an invalid value owner. Add a test case to TestSetScopeValueOwner to make sure nothing happens if the coin doesn't exist yet and an empty new value owner is provided. * [2137]: Unit tests on AtLeastOneAddrHasAccess and ValidateAtLeastOneAddrHasAccess. * [2137]: Update WithTransferAgents to set the value to a nil list of addresses instead of just a nil address. * [2137]: Fix a couple typos in the testlog stuff. * [2137]: Change the first arg of AddBurnCoinsStrs to moduleAddr for consistency. * [2137]: Fix a typo in a slices test case. * [2137]: Provide the scope id to emitMetadataNAVEvents. * [2137]: Remove SpendableCoin from the BankKeeper and fix a couple MockBankKeeper method comments. * [2137]: Remove the panic from RemoveScope and instead return an error. * [2137]: In TestUsedSignersMap, move the isUsed checks into the test runner. * [2137]: Swap out some TODOs involving metadata nav volume in the keeper scope tests. * [2137]: In the metadata keeper's GetNetAssetValue method, default the Volume to 1 if it's read in as zero from state (which means the field didn't exist when it was originally written. * [2137]: Update the stuff in the exchange module that needs to use the new Volume field in the metadata NAVs. * [2137]: Fix the WriteScope tests that should not be expecting a NAV in a bunch of them anymore. * [2137]: Fix the exchange tests that failed with the addition of Volume to the metadata NAV. * [2137]: Remove a couple unused proto imports. * [2137]: Regenerate stuff from the protos.
Description
Adds documentation to the nav command for scopes to explain the format and function
closes: #2134
closes: #2058
closes: #2030
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/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
Summary by CodeRabbit
New Features
metadata/scope navcommand, enhancing user understanding.GetCmdAddNetAssetValues, including new arguments and improved descriptions.EventSetNetAssetValueevent, specifying its attributes and purpose.Bug Fixes
Documentation
GetCmdAddNetAssetValuescommand, detailing net asset values and their representation.InitGenesisfunction to ensure valid volume values are used.