Use the bank module to keep track of scope value owners #2140
Conversation
…ed anywhere and anything trying to use the metadata keeper should define their own interface.
…eded when a scope is deleted.
…DOs for re-implementation.
…used when needed.
…eValueOwner endpoints to make it easier to change the stuff called in there.
… slice with nils removed (if it's got nils in it to begin with).
…DLinks since I couldn't actually use them.
…because the new way was sometimes returning an error when it shouldn't.
WalkthroughThe changes in this pull request primarily introduce the 'viridian' upgrade, which includes enhancements to the bank module for managing value ownership of scopes. New entries have been added to the changelog to document these changes. Additionally, modifications have been made to various application components, including the introduction of a generic Changes
Assessment against linked issues
Possibly related issues
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
CodeRabbit Configuration File (
|
…e 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.
…teScopes and GetScopeWithValueOwner.
… owner populated.
…ce it's used in there.
… 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.
…e in a few places.
…in an error in the UpdateValueOwnersCmd.
…f (and only works on a single scope), and refactor SetScopeValueOwners to require that all of the coins already exist.
…face and in the MockBankKeeper, impelement BlockedAddr, MintCoins BurnCoins and SendCoins.
…g the newValueOwner to prevent that lookup when the value owner is bad or blocked and include the burn error if there is one.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
x/metadata/types/scope.go (1)
534-538: New utility functionFindMissingPartiesadded.The new function provides a useful utility for comparing sets of parties. It effectively uses
provutils.FindMissingFuncandSamePartiersfor its implementation.Consider adding a brief example in the function's documentation to illustrate its usage and expected output. This would enhance the function's usability for other developers.
Example documentation addition:
// Example: // // required := []Party{{Address: "addr1", Role: PartyType_OWNER}, {Address: "addr2", Role: PartyType_CUSTODIAN}} // toCheck := []Party{{Address: "addr1", Role: PartyType_OWNER}, {Address: "addr3", Role: PartyType_AFFILIATE}} // missing := FindMissingParties(required, toCheck) // // missing will contain the Party{Address: "addr2", Role: PartyType_CUSTODIAN}x/metadata/keeper/scope.go (9)
70-82: Improved GetScope function with clear documentationThe changes to the
GetScopefunction are well-implemented. The updated function description clearly states that theValueOwnerAddressfield will always be empty, which is important for users of this function to understand.The use of
k.mustReadScopeBzis consistent with the changes seen inIterateScopes, promoting a standardized approach to reading scopes from storage.Consider adding a comment or log statement to warn developers if they attempt to access the
ValueOwnerAddressfield directly from the returnedScopeobject, as it will always be empty.
84-104: New helper functions for standardized scope readingThe addition of
readScopeBzandmustReadScopeBzfunctions is a good improvement. These helper functions standardize the process of reading scope data from bytes and ensure that theValueOwnerAddressis consistently set to an empty string.Consider adding error wrapping in the
readScopeBzfunction to provide more context when an error occurs. For example:err := k.cdc.Unmarshal(bz, &scope) if err != nil { return types.Scope{}, fmt.Errorf("failed to unmarshal scope: %w", err) }This will make debugging easier if unmarshaling errors occur.
126-140: Improved SetScope function with separate value owner handlingThe changes to the
SetScopefunction are well-implemented and align with the new approach of managing value owners separately. The addition of error handling forSetScopeValueOwneris appropriate and improves the robustness of the function.The use of a separate
writeScopeToStatefunction is a good design choice, likely improving testability and separation of concerns.Consider adding a comment explaining why
scope.ValueOwnerAddressis set to an empty string after callingSetScopeValueOwner. This will help future developers understand the reasoning behind this step.
204-226: New functions for retrieving scope value ownersThe addition of
GetScopeValueOwnerandGetScopeValueOwnersfunctions is excellent. These functions are crucial for the new value owner management system, providing a clean interface to retrieve value owners for scopes.The error handling in
GetScopeValueOwnersis particularly well-implemented, usingerrors.Jointo aggregate errors for multiple scopes.In the
GetScopeValueOwnersfunction, consider using a pre-allocated slice forrvto potentially improve performance:rv := make(types.AccMDLinks, 0, len(ids))This avoids potential reallocation as the slice grows.
228-284: Comprehensive new function for setting scope value ownerThe
SetScopeValueOwnerfunction is a critical addition for managing scope value owners. It handles various scenarios including setting, minting, and burning of scope value owner coins with thorough error checking and handling.The function covers different cases well:
- Burning coins when there's no new value owner
- Minting coins when there's no current value owner
- Transferring coins between owners
The error messages are descriptive and will aid in debugging.
Consider adding a comment explaining the significance of using the module account (
k.moduleAddr) for minting and burning operations. This will help future developers understand the design choice.
286-324: Efficient new function for setting multiple scope value ownersThe
SetScopeValueOwnersfunction is a well-implemented addition for handling bulk updates of value owners for multiple scopes. The function includes thorough error checking, especially for the new value owner address.The aggregation of coins before sending is a good optimization, reducing the number of transactions needed.
Consider adding a check to ensure that
newValueOwneris not an empty string at the beginning of the function. This would prevent unnecessary processing if an invalid input is provided:if newValueOwner == "" { return fmt.Errorf("new value owner address cannot be empty") }
Line range hint
427-524: Enhanced ValidateWriteScope function with value owner handlingThe changes to the
ValidateWriteScopefunction are well-implemented and align with the new value owner management system. The introduction of the "onlyChangeIsValueOwner" check allows for more flexible validation rules, which is a good improvement.The error handling has been enhanced, making the function more robust and easier to debug. The separation of value owner validation from other validations is a good design choice.
Consider extracting the logic for checking "onlyChangeIsValueOwner" into a separate helper function. This would improve readability and make the logic easier to test independently. For example:
func isOnlyChangeValueOwner(existing, proposed *types.Scope) bool { if existing == nil || len(existing.ValueOwnerAddress) == 0 || existing.ValueOwnerAddress == proposed.ValueOwnerAddress { return false } proposedCopy := proposed proposedCopy.ValueOwnerAddress = existing.ValueOwnerAddress return existing.Equals(proposedCopy) }
799-822: New function for validating bulk value owner updatesThe
ValidateUpdateValueOwnersfunction is a well-implemented addition for validating bulk updates of value owners across multiple scopes. It provides comprehensive checks, including validation for empty links and proposed value owner.The use of
ValidateScopeValueOwnersSignersensures consistent validation with other parts of the system. The error handling is thorough and provides clear, actionable error messages.Consider adding a check for the maximum number of links that can be processed in a single call to prevent potential DoS attacks or excessive resource usage. For example:
const maxLinksPerUpdate = 100 // Adjust this value as needed if len(links) > maxLinksPerUpdate { return nil, fmt.Errorf("too many scopes in a single update: got %d, max allowed is %d", len(links), maxLinksPerUpdate) }
Line range hint
870-895: Improved SetNetAssetValue function with Volume handlingThe changes to the
SetNetAssetValuefunction are well-implemented. The addition of a check and default value for the Volume field ensures consistency with the new NAV structure and maintains backward compatibility.The use of a typed event (
NewEventSetNetAssetValue) for setting NAV improves the system's observability, which is crucial for monitoring and auditing purposes.The error handling has been enhanced, making the function more robust and easier to debug.
Consider adding a log statement when the Volume is automatically set to 1. This will help with debugging and monitoring the frequency of this default behavior:
if netAssetValue.Volume < 1 { netAssetValue.Volume = 1 k.Logger(ctx).Info("NAV Volume automatically set to 1", "scopeID", scopeID, "priceDenom", netAssetValue.Price.Denom) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- proto/provenance/metadata/v1/scope.proto (1 hunks)
- x/exchange/keeper/commitments_test.go (6 hunks)
- x/exchange/keeper/fulfillment.go (5 hunks)
- x/exchange/keeper/fulfillment_test.go (19 hunks)
- x/exchange/keeper/mocks_test.go (9 hunks)
- x/metadata/client/cli/cli_test.go (3 hunks)
- x/metadata/client/cli/tx.go (1 hunks)
- x/metadata/keeper/genesis.go (1 hunks)
- x/metadata/keeper/msg_server.go (10 hunks)
- x/metadata/keeper/msg_server_test.go (20 hunks)
- x/metadata/keeper/scope.go (17 hunks)
- x/metadata/types/scope.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- proto/provenance/metadata/v1/scope.proto
- x/metadata/client/cli/tx.go
- x/metadata/keeper/genesis.go
🧰 Additional context used
📓 Learnings (5)
x/exchange/keeper/fulfillment.go (1)
Learnt from: SpicyLemon PR: provenance-io/provenance#2140 File: x/exchange/keeper/fulfillment.go:386-387 Timestamp: 2024-10-02T02:42:41.151Z Learning: `MetadataAddressFromDenom(denom)` handles the prefix internally, so it's unnecessary to trim the prefix from `denom` before calling it.x/exchange/keeper/mocks_test.go (1)
Learnt from: SpicyLemon PR: provenance-io/provenance#2140 File: x/exchange/keeper/mocks_test.go:427-431 Timestamp: 2024-10-02T03:16:46.324Z Learning: In Go unit tests, repetitive code is acceptable; refactoring for DRY is not necessary.x/metadata/keeper/msg_server.go (2)
Learnt from: SpicyLemon PR: provenance-io/provenance#2140 File: x/metadata/keeper/msg_server.go:206-209 Timestamp: 2024-10-02T03:54:36.749Z Learning: In the `DeleteScopeOwner` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.Learnt from: SpicyLemon PR: provenance-io/provenance#2140 File: x/metadata/keeper/msg_server.go:106-109 Timestamp: 2024-10-02T03:53:30.381Z Learning: In the `AddScopeDataAccess` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.x/metadata/keeper/msg_server_test.go (2)
Learnt from: SpicyLemon PR: provenance-io/provenance#2140 File: x/metadata/keeper/msg_server_test.go:1225-1228 Timestamp: 2024-10-02T03:37:18.486Z Learning: When test cases are run using `s.T().Run` instead of `s.Run`, use `require` with `t` instead of `s.Require()`, to ensure that assertions apply to the correct test instance.Learnt from: SpicyLemon PR: provenance-io/provenance#2140 File: x/metadata/keeper/msg_server_test.go:1248-1251 Timestamp: 2024-10-02T03:39:04.537Z Learning: In subtests, it's acceptable to use `require` and `assert` functions directly instead of `s.Require()` and `s.Assert()` methods.x/metadata/keeper/scope.go (1)
Learnt from: SpicyLemon PR: provenance-io/provenance#2140 File: x/metadata/keeper/scope.go:115-122 Timestamp: 2024-10-02T18:00:18.803Z Learning: `PopulateScopeValueOwner` is designed to handle errors internally and should not return an error. Returning an error would complicate callers unnecessarily, as errors from `GetScopeValueOwner` indicate there is no value owner for the scope.
🔇 Additional comments (62)
x/metadata/types/scope.go (2)
9-10: LGTM: New import added for provutils package.The addition of the
provutilsimport is appropriate as it's used in the newFindMissingPartiesfunction.
49-57: Improved validation logic for ScopeId and SpecificationId.The changes to
ValidateBasicmethod enhance the validation process by using more specific validation methods. This improves code readability and maintainability.To ensure the new validation methods cover all necessary checks, please run the following:
x/metadata/keeper/scope.go (8)
23-24: Improved scope reading and populationThe changes to
IterateScopesfunction look good. The use ofk.mustReadScopeBzsuggests a more standardized approach to reading scopes from storage. The addition ofk.PopulateScopeValueOwnerensures that the value owner is properly set for each scope during iteration.
106-113: New convenient function for getting scope with value ownerThe addition of the
GetScopeWithValueOwnerfunction is a great improvement. It provides a convenient way to retrieve a scope along with its value owner in a single function call. This function is consistent with the new approach of separating scope data from value owner data, while still allowing easy access to both when needed.
115-122: Critical new function for populating scope value ownerThe
PopulateScopeValueOwnerfunction is a crucial addition for the new approach of managing value owners separately from scopes. It efficiently looks up and sets theValueOwnerAddressin the provided scope.The error handling is appropriate, setting an empty string if there's an error or no value owner. This ensures that the
ValueOwnerAddressfield is always in a consistent state.This function is central to the new value owner management system and its implementation looks solid.
172-201: Improved RemoveScope function with value owner handlingThe changes to the
RemoveScopefunction are well-implemented. The addition of error handling and the call toSetScopeValueOwnerto remove the value owner are consistent with the new value owner management approach.The improved error handling, particularly for the value owner removal step, makes the function more robust and easier to debug.
These changes enhance the reliability and consistency of scope removal operations.
Line range hint
529-589: Improved ValidateDeleteScope function with value owner handlingThe changes to the
ValidateDeleteScopefunction are well-implemented and align with the new value owner management system. The addition of value owner validation usingValidateScopeValueOwnersSignersensures consistent validation across different functions.The error handling has been enhanced, making the function more robust and easier to debug. The separation of value owner validation from other validations is a good design choice.
These changes improve the reliability and consistency of scope deletion validation, especially with regard to value owner management.
Line range hint
765-791: Enhanced ValidateUpdateScopeOwners with smart contract signer validationThe changes to the
ValidateUpdateScopeOwnersfunction are well-implemented. The addition of smart contract signer validation usingvalidateSmartContractSignersenhances the security of scope owner updates.The error handling has been improved, making the function more robust and easier to debug. The separation of different validation steps (roles, provenance, signers, smart contracts) is a good design choice that improves the function's readability and maintainability.
These changes strengthen the validation process for updating scope owners, particularly in scenarios involving smart contracts.
842-869: Well-implemented function for retrieving Net Asset ValueThe
GetNetAssetValuefunction is a solid addition for retrieving Net Asset Value (NAV) data for a given scope and price denomination. It includes thorough error handling for metadata address conversion and unmarshaling, providing clear and actionable error messages.The fallback mechanism for older NAV records without a Volume field is a thoughtful inclusion, ensuring backward compatibility with previously stored data. This demonstrates good consideration for system evolution and data migration.
This function provides a robust and backward-compatible way to retrieve NAV data, which will be valuable for various financial operations within the system.
Line range hint
1-958: Overall assessment of changes in scope.goThe changes in
x/metadata/keeper/scope.gorepresent a significant enhancement to the metadata module, particularly in the areas of value owner management and Net Asset Value (NAV) handling. The key improvements include:
- Separation of value owner management from scope management, improving modularity and flexibility.
- Introduction of new functions for bulk operations on value owners, enhancing efficiency for large-scale operations.
- Improved error handling and validation across multiple functions, increasing the robustness of the system.
- Addition of NAV-related functions, expanding the financial capabilities of the system.
- Consistent use of typed events, improving system observability.
These changes collectively contribute to a more modular, efficient, and maintainable codebase. The careful handling of backward compatibility, particularly in NAV-related functions, demonstrates thoughtful consideration of system evolution.
While the changes are substantial, they appear to be well-implemented and aligned with good software engineering practices. As the system complexity has increased, it may be beneficial to consider adding more comprehensive unit tests and documentation to ensure long-term maintainability and ease of understanding for new developers.
x/exchange/keeper/mocks_test.go (11)
7-7: LGTM: New imports added for additional functionality.The new imports for "strings", "provutils", and "metadata/types" are appropriate additions to support the new functionality in this file.
Also applies to: 14-14, 18-18
836-836: LGTM: New field added to MockMarkerKeeper for net asset values.The addition of
GetNetAssetValueMapto the MockMarkerKeeper struct is appropriate for simulating the storage and retrieval of net asset values in the mock implementation.
857-857: LGTM: New field added to MarkerCalls for tracking GetNetAssetValue calls.The addition of
GetNetAssetValueto the MarkerCalls struct is consistent with the existing pattern and will allow for proper tracking of these method calls in tests.
864-864: LGTM: Updated assertGetNetAssetValueCalls to match new field name.The function has been correctly updated to use "GetNetAssetValue" instead of "GetNetAssetValueArgs", maintaining consistency with the changes in the MarkerCalls struct.
875-881: LGTM: New WithGetNetAssetValue method added to MarkerCalls.The new WithGetNetAssetValue method is a good addition that allows for easy recording of GetNetAssetValue calls in the mock implementation. It follows the established pattern for similar methods in this struct.
924-929: LGTM: New MockMetadataKeeper struct and methods added.The addition of MockMetadataKeeper and its associated methods (NewMockMetadataKeeper, WithAddSetNetAssetValuesErrors, WithGetNetAssetValueErrors, WithGetNetAssetValueResult, AddSetNetAssetValues, GetNetAssetValue) is well-structured and consistent with the existing mock implementations in this file. These additions will allow for proper testing of metadata-related functionality in the exchange module.
Also applies to: 949-1001
1003-1022: LGTM: New assertion methods added for MockMetadataKeeper.The addition of assertMDAddSetNetAssetValues, assertMDGetNetAssetValueCalls, and assertMetadataKeeperCalls methods is consistent with the existing assertion methods in this file. These will facilitate thorough testing of the metadata keeper functionality in the exchange module.
1024-1032: LGTM: New methods added to MetadataCalls for recording mock calls.The addition of WithAddSetNetAssetValues and WithGetNetAssetValue methods to the MetadataCalls struct is consistent with the existing pattern in this file. These methods will facilitate easy recording of mock calls during tests.
937-942: LGTM: New MDAddSetNetAssetValuesArgs struct and methods added.The addition of MDAddSetNetAssetValuesArgs struct and its associated methods (NewMDAddSetNetAssetValuesArgs, String, mdAddNAVArgsString) is well-structured and consistent with similar implementations in this file. These additions will facilitate proper representation and formatting of arguments for the AddSetNetAssetValues method in tests.
Also applies to: 1034-1058
947-947: LGTM: New MDGetNetAssetValueArgs struct and methods added.The addition of MDGetNetAssetValueArgs struct and its associated methods (NewMDGetNetAssetValueArgs, MetadataDenom, PriceDenom, String, mdNAVString) is well-structured and consistent with similar implementations in this file. The use of provutils.Pair for efficient storage is a good choice. These additions will facilitate proper representation and formatting of arguments for the GetNetAssetValue method in tests.
Also applies to: 1060-1083
944-944: LGTM: New MDGetNetAssetValueResult struct and methods added.The addition of MDGetNetAssetValueResult struct and its associated methods (NewMDGetNetAssetValueResult, Nav, Err) is well-structured and consistent with similar implementations in this file. The use of provutils.Pair for efficient storage is appropriate. The Err method correctly handles the conversion of the stored string to an error or nil. These additions will facilitate proper handling and representation of GetNetAssetValue results in tests.
Also applies to: 1085-1101
x/metadata/keeper/msg_server_test.go (20)
76-87: LGTM: New helper function for creating test addresses.The
newAddrfunction is a well-implemented helper for creating consistent test addresses. It handles different input string lengths and ensures the resulting address is always of a valid length (20 or 32 bytes).
91-104: LGTM: New helper function for setting up test accounts.The
storeUserAccountfunction is a well-implemented helper for creating or updating test accounts. It ensures consistent account states by setting the sequence number and public key as needed.
108-110: LGTM: New helper function for creating accounts from public keys.The
createAccountFromPubKeyfunction provides a simple way to create test accounts from public keys. It's a good abstraction that will improve test readability and consistency.
114-116: LGTM: New helper function for setting up test accounts without public keys.The
setUserAccountfunction is a useful helper for creating or updating test accounts when only the address is important. It simplifies test setup in cases where public keys are not needed.
120-122: LGTM: New helper function for creating named test accounts.The
setNamedUserAccountfunction is a valuable addition that allows for creating test accounts with human-readable names. This will improve test readability and make it easier to track different accounts in complex test scenarios.
126-131: LGTM: New helper function for creating simulated smart contract accounts.The
setNamedSmartContractAccountfunction is a useful addition for creating test accounts that simulate smart contract accounts. This will be valuable for testing scenarios involving smart contracts without the need to actually deploy them.
134-146: LGTM: New helper function for creating deterministic UUIDs.The
newUUIDfunction is a well-implemented helper for generating deterministic UUIDs in tests. It ensures consistency and reproducibility, which is crucial for reliable testing of UUID-related functionality.
149-162: LGTM: New helper functions for creating deterministic metadata addresses.The
scopeID,sessionID, andscopeSpecIDfunctions are well-implemented helpers for generating consistent and deterministic metadata addresses. These will improve test readability and reduce the chance of errors when creating addresses for scopes, sessions, and scope specifications.
165-169: LGTM: New helper function for converting typed events.The
untypeEventfunction is a useful helper for converting typed events to sdk.Events in tests. It improves error handling and simplifies the testing process for event-related functionality.
172-176: LGTM: New helper function for converting bech32 strings to addresses.The
fromBech32function is a useful helper for converting bech32 strings to sdk.AccAddress in tests. It improves error handling and simplifies the testing process for address-related functionality.
180-186: LGTM: New helper function for creating non-WASM test accounts.The
MakeNonWasmAccountsfunction is a valuable addition for creating test accounts that are not recognized as WASM accounts. This will be useful for testing scenarios where the distinction between WASM and non-WASM accounts is important.
1358-1360: LGTM: Minor improvements to test account setup.The addition of the
MakeNonWasmAccountscall in the TestAddAndDeleteScopeDataAccess function ensures that the test accounts are properly set up as non-WASM accounts. This is a good improvement to the test setup.
1406-1442: LGTM: Improved test setup for scope owners.The changes in the TestAddAndDeleteScopeOwners function, including the addition of the
MakeNonWasmAccountscall, ensure that the test accounts are properly set up as non-WASM accounts. This improves the accuracy of the test scenarios.
1695-1699: LGTM: Improved assertions in MigrateValueOwner tests.The changes in the TestMigrateValueOwner function include updated error messages and improved assertions. These changes make the tests more accurate and easier to debug, which is a positive improvement.
Line range hint
1852-2095: LGTM: New comprehensive tests for AddNetAssetValues functionality.The new TestAddNetAssetValue function is a valuable addition to the test suite. It provides comprehensive coverage for the AddNetAssetValues functionality, including various scenarios such as error cases and successful operations. This will help ensure the reliability of the net asset value management features.
71-73: Verify the implementation of AssertEqualEvents in the assertions package.The new AssertEqualEvents function is a good addition for comparing events in tests. It improves readability and maintainability of event-related tests.
To verify the implementation of the AssertEqualEvents function in the assertions package, run the following script:
#!/bin/bash # Description: Check for the implementation of AssertEqualEvents function in the assertions package # Test: Search for AssertEqualEvents function definition echo "Searching for AssertEqualEvents function:" rg --type go 'func AssertEqualEvents' ./testutil/assertions/
760-1074: Comprehensive improvements to DeleteScope testing.The TestDeleteScope function has been significantly expanded with new helper functions and test cases. This improves the test coverage for the DeleteScope functionality, including various scenarios such as deleting scopes with different configurations of records, sessions, and value owners.
While these changes are generally positive, it's important to ensure that all new test cases are properly implemented and cover all intended scenarios.
To verify the completeness of the test cases, run the following script:
#!/bin/bash # Description: Check for the implementation of all test cases in TestDeleteScope # Test: Count the number of test cases echo "Number of test cases in TestDeleteScope:" rg --type go 'name:.*' x/metadata/keeper/msg_server_test.go -A 10 | rg 'setup:' | wc -l # Test: List all test case names echo "Test case names:" rg --type go 'name:.*' x/metadata/keeper/msg_server_test.go -A 10 | rg 'setup:'
188-757: Comprehensive improvements to WriteScope testing.The TestWriteScope function has been significantly expanded with new helper functions and test cases. This improves the test coverage for the WriteScope functionality, including various scenarios such as value owner changes, marker interactions, and smart contract interactions.
While these changes are generally positive, it's important to ensure that all new test cases are properly implemented and cover all intended scenarios.
To verify the completeness of the test cases, run the following script:
#!/bin/bash # Description: Check for the implementation of all test cases in TestWriteScope # Test: Count the number of test cases echo "Number of test cases in TestWriteScope:" rg --type go 'name:.*' x/metadata/keeper/msg_server_test.go | wc -l # Test: List all test case names echo "Test case names:" rg --type go 'name:.*' x/metadata/keeper/msg_server_test.go
Line range hint
1546-1605: Comprehensive improvements to UpdateValueOwners testing.The TestUpdateValueOwners function has been significantly expanded with new test cases. This improves the test coverage for the UpdateValueOwners functionality, including various scenarios such as updating value owners for multiple scopes and handling different error conditions.
While these changes are generally positive, it's important to ensure that all new test cases are properly implemented and cover all intended scenarios.
To verify the completeness of the test cases, run the following script:
#!/bin/bash # Description: Check for the implementation of all test cases in TestUpdateValueOwners # Test: Count the number of test cases echo "Number of test cases in TestUpdateValueOwners:" rg --type go 'name:.*' x/metadata/keeper/msg_server_test.go -A 20 | rg 'starters:' | wc -l # Test: List all test case names echo "Test case names:" rg --type go 'name:.*' x/metadata/keeper/msg_server_test.go -A 20 | rg 'starters:'
47-47: Verify the implementation of new setup methods.The changes to the SetupTest function introduce new methods for creating the test context and user accounts. While these changes appear to be improvements, it's important to ensure that the
FreshCtxandcreateAccountFromPubKeymethods are properly implemented and tested.To verify the implementation of these methods, run the following script:
Also applies to: 50-50, 54-54
✅ Verification successful
Implementation Verified Successfully.
BothFreshCtxandcreateAccountFromPubKeymethods are correctly implemented in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the implementation of FreshCtx and createAccountFromPubKey methods # Test: Search for FreshCtx function definition echo "Searching for FreshCtx function:" rg --type go 'func.*FreshCtx' # Test: Search for createAccountFromPubKey method definition echo "Searching for createAccountFromPubKey method:" rg --type go 'func \(.*\) createAccountFromPubKey'Length of output: 921
x/metadata/client/cli/cli_test.go (4)
1579-1579: Removed unused code.The line
expOut: []string{"scope_uuids: []", "total: \"0\""},has been removed. This appears to be a cleanup of unused test expectations.
2236-2236: Fixed indentation.The line has been indented correctly to align with the surrounding code structure.
2383-2383: Fixed indentation.Similar to the previous change, this line has been properly indented to maintain consistent code structure.
Line range hint
1-4639: Summary of changesThe changes in this file are minor and consist of:
- Removal of an unused test expectation in the
TestOwnershipCmdfunction.- Two instances of fixing indentation to improve code readability and consistency.
These changes do not affect the functionality of the tests and appear to be part of a code cleanup effort.
Overall, the modifications improve the code quality slightly without introducing any new features or altering existing behavior. The changes are safe and can be approved.
x/exchange/keeper/fulfillment.go (5)
6-6: Importing the "strings" package is necessary.The
stringspackage is required for string manipulation functions used in the code, such asHasPrefixandTrimPrefix.
17-17: Including themetadatatypespackage is appropriate.The import of
metadatatypesallows for interaction with the metadata module, which is integral to the new functionality introduced.
425-427: Definition ofemitMarkerNAVEventsfunction is appropriate.The new function
emitMarkerNAVEventscorrectly encapsulates the emission of NAV events for the marker module, ensuring clarity and reusability.
435-444: Addition ofemitMetadataNAVEventsfunction is well-implemented.The newly added
emitMetadataNAVEventsfunction properly handles the emission of NAV events for the metadata module, enhancing modularity and maintainability.
445-459:GetNavfunction correctly retrieves NAVs from both metadata and marker modules.The updated
GetNavfunction now checks if theassetsDenomhas the metadata prefix and retrieves the NAV from the appropriate module accordingly. This enhancement ensures accurate NAV retrieval based on the asset type.x/metadata/keeper/msg_server.go (1)
15-15: Appropriate Addition of Import StatementThe addition of
markertypesimport is necessary for handling transfer agents, which aligns with the PR objectives and enhances the functionality.x/exchange/keeper/commitments_test.go (7)
15-15: Import 'metadatatypes' added correctlyThe addition of the
metadatatypesimport is appropriate for handling metadata interactions in the tests.
1815-1816: Initialize 'scopeID1' and 'scopeID2' for metadata testsThe initialization of
scopeID1andscopeID2enhances the test cases by incorporating scope-related metadata handling.
1821-1821: Add 'mdKeeper' field to test structIncluding a
MockMetadataKeeperin the test struct allows mocking metadata interactions, which is necessary for the extended test scenarios.
1827-1827: Add 'expMDCalls' field to test structAdding
expMDCallshelps in asserting expected calls to the metadata keeper during the tests, ensuring accurate verification of metadata interactions.
1934-1937: Verify use of 'scopeID.Coin()' in NAVsThe usage of
scopeID1.Coin()andscopeID2.Coin()in theNavsslice may require verification. Ensure thatscopeID.Coin()returns a validsdk.Coinwith the intended denomination.Does the
Coin()method onscopeIDproduce a coin with the correct denomination? If not, consider using a method or conversion that accurately represents the scope ID as a coin denomination in the NAVs.
2179-2181: Initialize 'mdKeeper' if nilThe conditional initialization of
mdKeeperensures that aMockMetadataKeeperis available for the tests, preventing potential nil pointer dereferences.
2219-2220: Assert metadata keeper callsThe addition of
s.assertMetadataKeeperCalls(tc.mdKeeper, tc.expMDCalls, "SettleCommitments")correctly validates the expected interactions with the metadata keeper during theSettleCommitmentstest.x/exchange/keeper/fulfillment_test.go (4)
10-10: Addition of import for metadata typesThe import of
metadatatypesallows the test suite to utilize metadata-related types, which is necessary for the new tests involving scopes and metadata interactions.
Line range hint
1734-1754: Integration of MockMetadataKeeper and related variablesIntroducing
mdKeeper *MockMetadataKeeperandexpMDCalls MetadataCallsin the test suite enhances the ability to simulate and verify interactions with the metadata module within the exchange keeper tests.
2040-2082: New test case 'one ask one bid: scope' effectively tests scope asset settlementThe addition of the test case
"one ask one bid: scope"accurately tests the settlement process for orders involving scoped assets. It aligns with the PR objectives by ensuring that scope assets are correctly handled in the exchange module.
Line range hint
2622-2717: Expansion of TestKeeper_GetNav to include metadata NAV retrievalThe enhanced
TestKeeper_GetNavfunction now includes test cases for retrieving net asset values from both the marker and metadata keepers. This ensures comprehensive coverage for assets represented as scopes, verifying that NAV retrieval functions correctly across different asset types.
There was a problem hiding this comment.
Working through this PR the NAV system design issues are pretty clear ... there should really be one catalog of values ... it might make sense in a future update to merge all that stuff into the marker module vs having it with different types in the metadata module. feel like this would remove a lot of extra code especially in the exchange module which has to know about what kind of denom it is working with and how that works which feels wrong.
In a related fashion perhaps a v2 on the metadata module can streamline out all the value owner facilitated transfer pieces ... there are a lot of bindings added that bring checks on source/destination which would probably be cleared up to some extent if we left that to the bank side of the system and simply required the coin to be sent in with the request when burning/destroying a scope. On the whole the approach here of being seamless is by far the best way to handle this kind of change since it keeps the breaking changes to a minimum.
Overall this feels like a significant improvement in tracking value for digital assets on chain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/proto-docs.md (1)
10737-10737: Approve changes with minor suggestions for improvementThe updated description for the
value_owner_addressfield aligns well with the PR objectives and provides a detailed explanation of the new value owner tracking system. Great job on including information about how the value owner is tracked, how it can be changed, and the behavior during WriteScope operations.To further improve the documentation:
Consider breaking the long description into bullet points for easier readability. This will help users quickly grasp the key points.
Fix the line break formatting. The
<br>tags are visible in the rendered markdown, which isn't ideal.Here's a suggested revision:
| `value_owner_address` | [string](#string) | | The address that controls the value associated with this scope. • The value owner is tracked by the bank module using a coin with the denom "nft/<scope_id>". • The value owner can be changed using WriteScope or anything that transfers funds, e.g. MsgSend. • During WriteScope: - If this field is empty, it indicates that there should not be a change to the value owner. Once a scope has a value owner, it will always have one (until it's deleted). - If this field has a value, the existing value owner will be looked up, and: * If there's already an existing value owner, they must be a signer, and the coin will be transferred to the new value owner. * If there isn't yet a value owner, the coin will be minted and sent to the new value owner. • If the scope already exists, the owners must be signers (just like changing other fields). • If it's a new scope, there's no special signer limitations related to the value owner. |This format maintains all the important information while improving readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
x/metadata/types/scope.pb.gois excluded by!**/*.pb.gox/quarantine/genesis.pb.gois excluded by!**/*.pb.gox/quarantine/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
- client/docs/swagger-ui/swagger.yaml (24 hunks)
- docs/proto-docs.md (1 hunks)
- proto/cosmos/quarantine/v1beta1/genesis.proto (0 hunks)
- proto/cosmos/quarantine/v1beta1/tx.proto (0 hunks)
💤 Files with no reviewable changes (2)
- proto/cosmos/quarantine/v1beta1/genesis.proto
- proto/cosmos/quarantine/v1beta1/tx.proto
🧰 Additional context used
🔇 Additional comments (2)
client/docs/swagger-ui/swagger.yaml (2)
Line range hint
14079-87513: Summary of changes to swagger.yamlThe primary change in this file is the comprehensive update to the
value_owner_addressfield description. This update has been consistently applied across multiple locations in the swagger.yaml file. The new description accurately reflects the changes introduced in this PR, particularly the use of the bank module for tracking value ownership.These changes in the API documentation are crucial for developers interacting with the Provenance blockchain, as they provide clear guidance on how value ownership is managed and the constraints involved in modifying it.
14079-14098: Comprehensive update tovalue_owner_addressfield descriptionThe description for the
value_owner_addressfield has been significantly expanded and clarified across multiple locations in the swagger.yaml file. This update provides crucial information about the new implementation of value ownership tracking and management. Key points include:
- Value ownership is now tracked by the bank module using a coin with the denomination "nft/<scope_id>".
- The value owner can be changed using WriteScope or any transaction that transfers funds (e.g., MsgSend).
- Detailed behavior during WriteScope operations, including:
- Empty field indicating no change to the value owner
- Existing value owner must be a signer for transfers
- New coin minting for new value owners
- Signer requirements for existing and new scopes
This update aligns with the PR objectives of enhancing the management of value owner records and utilizing the bank module for more efficient tracking.
To ensure consistency across the codebase, please run the following script:
Also applies to: 17167-17186, 18028-18047, 18895-18914, 20919-20938, 21771-21790, 22657-22676, 23532-23551, 24418-24437, 25293-25312, 26164-26183, 27050-27069, 27877-27896, 29187-29206, 30054-30073, 30929-30948, 31800-31819, 80011-80029, 83661-83680, 84357-84375, 84534-84553, 86212-86230, 86443-86462, 87494-87513
✅ Verification successful
Verified: Consistent Usage of
nft/<scope_id>andvalue_owner_addressAcross CodebaseNo inconsistencies found in the usage of
nft/<scope_id>denominations or in handlingvalue_owner_addressacross the codebase. All instances align with the updated descriptions and implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent usage of "nft/<scope_id>" denomination across the codebase echo "Checking for consistent usage of 'nft/<scope_id>' denomination:" rg --type go -i 'nft/.*scope.*id' echo "\nChecking for potential inconsistencies in value owner handling:" rg --type go -i 'value.*owner'Length of output: 131703
Description
closes: #2137
This PR takes the value owner records out of the scopes and uses the bank module to keep track of them by minting a single coin with the denom
"nft/<scope id>". The account that has that coin is the value owner of the scope. This allows a scope's value owner to be changed using anything that can transfer funds, e.g. a bankMsgSendor even with the exchange module (also withWriteScopestill).The metadata module is bumped to v4 and a migration is added that will migrate all the value owner data into the bank module. The
viridianupgrade is also created.This PR also updates the exchange module to watch for scopes being traded in order to decide if the NAV info should be updated in the marker or metadata modules. As of right now, the upgrade will prune IBC expired consensus state, run module migrations (including the metadata module one), and remove inactive validator delegations.
In the marker module, a new
IsMarkerAccountis added to the keeper allowing that check to happen without having to read in the entire account. Also, send restrictions now account for the possibility of having multiple transfer agents so that the multiple signers of, e.g.WriteScopecan be used for deposit or withdraw permissions as needed.As part of this change, the
Ownershipquery will no longer return scopes that have the provided address as the value owner unless they are also in the scope'sOwnerslist. I.e. theOwnershipquery is now only concerned with theOwnersfield in scopes. TheValueOwnershiphasn't changed though (other than making sure it still works).Also, the
WriteScopeendpoint now treats thescope.value_owner_addressdifferently. Now, if it's provided as an empty string, that indicates that there is no desired change to the value owner of the scope. That means that it won't try to do any lookups or signature verification that would happen if the value owner were changing. Once a scope has a value owner, the only way for it to stop having a value owner is by deleting the scope (which also requires a signature from the value owner).Lastly, an authz grant for the
MsgWriteScopemessage type no longer applies to theUpdateValueOwnersorMigrateValueOwnerendpoints. Grants for those endpoints must be given individually.There's still some related work to do here:
I added someDone in this PR.TODO[2160]comments that will need to be handled once Improve metadata scope nav cli command documentation #2160 has merged. If that merges before this one, I'll handle them in this PR, otherwise, I'll need another PR to do them.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
New Features
Improvements
WriteScopeendpoint to streamline value ownership changes.value_owner_addressfield in the API.Bug Fixes
Documentation