feat: Marketplace & Auction Adjustments #589
Conversation
…d remove authorized cw20 contracts functions, unit tests
WalkthroughThe pull request introduces several changes to the Changes
Assessment against linked issues
Possibly related issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 9
🧹 Outside diff range and nitpick comments (2)
contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (2)
204-243: Consistency in handlingnon_authorizedaddressFor consistency and to prevent potential errors, consider defining
non_authorizedas anAndrAddrand retrieving the raw address similarly to the authorized addresses.Apply this diff to make the change:
// Check that a non-authorized address doesn't have permission - let non_authorized = "non_authorized_cw20".to_string(); + let non_authorized = AndrAddr::from_string("non_authorized_cw20"); let permission = - ADOContract::get_permission(deps.as_ref().storage, SEND_CW20_ACTION, non_authorized) + ADOContract::get_permission( + deps.as_ref().storage, + SEND_CW20_ACTION, + non_authorized.get_raw_address(&deps.as_ref()).unwrap() + ) .unwrap(); assert_eq!(permission, None);
333-333: Reduce code duplication by refactoring initializationThe repeated initialization of
Some(vec![AndrAddr::from_string(MOCK_CW20_CONTRACT)])in multiple tests can be refactored into a helper function or constant to improve maintainability and reduce redundancy.Also applies to: 450-450, 538-538, 600-600, 625-625, 822-822
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
- contracts/non-fungible-tokens/andromeda-marketplace/Cargo.toml (1 hunks)
- contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs (6 hunks)
- contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (12 hunks)
- packages/andromeda-non-fungible-tokens/src/marketplace.rs (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/non-fungible-tokens/andromeda-marketplace/Cargo.toml
🔇 Additional comments (7)
packages/andromeda-non-fungible-tokens/src/marketplace.rs (1)
44-62: Verify access control for new execute messages.Ensure that the new execute messages (
AuthorizeCw20Contract,DeauthorizeCw20Contract,AuthorizeTokenContract,DeauthorizeTokenContract) are properly restricted to authorized entities, such as the contract owner. This is crucial to prevent unauthorized modifications to the list of authorized addresses.You can verify the access controls by examining the handler functions:
contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (6)
3-5: Imports for new functionality are appropriateThe added imports for
AuthorizedAddressesResponse,Cw20HookMsg,Cw721HookMsg,ExecuteMsg,InstantiateMsg,QueryMsg, andStatusare necessary for handling the new authorization features in the marketplace tests.
8-11: Imports for permissions and rates are correctly includedThe imports from
ado_basefor permissioning and rates are required to manage permissions and rate calculations within the tests.
15-15: Importing action constants is appropriateImporting
Asset,SEND_CW20_ACTION, andSEND_NFT_ACTIONis necessary for referencing actions in permission settings and asset handling.
37-37: Importing contract functions is necessaryThe imports of
execute,instantiate, andqueryfromcontractare required for executing and testing contract functions.
109-115: Updatedinitfunction includes authorization parametersIncluding
authorized_cw20_addressesandauthorized_token_addressesin theinitfunction aligns with the new requirements for multiple authorized addresses. Ensure that all calls toinitin the test suite are updated to reflect the new parameters.
1030-1060: Verify formatting ofexpirationin assertion attributesEnsure that
expirationimplementsDisplaycorrectly, so thatformat!("{}", expiration)produces the expected string in the assertion. IfExpirationdoes not implementDisplay, consider formatting it appropriately for the test.
contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs
Outdated
Show resolved
Hide resolved
contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs
Outdated
Show resolved
Hide resolved
| fn execute_deauthorize_cw20_contract( | ||
| deps: DepsMut, | ||
| info: MessageInfo, | ||
| cw20_address: AndrAddr, | ||
| ) -> Result<Response, ContractError> { | ||
| let contract = ADOContract::default(); | ||
| ensure!( | ||
| contract.is_contract_owner(deps.storage, info.sender.as_str())?, | ||
| ContractError::Unauthorized {} | ||
| ); | ||
|
|
||
| let addr = cw20_address.get_raw_address(&deps.as_ref())?; | ||
|
|
||
| ADOContract::remove_permission(deps.storage, SEND_CW20_ACTION, addr.to_string())?; | ||
|
|
||
| Ok(Response::default().add_attributes(vec![ | ||
| attr("action", "deauthorize_cw20_contract"), | ||
| attr("cw20_address", addr), | ||
| ])) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate deauthorization functions to eliminate duplication
Similarly, the functions execute_deauthorize_cw20_contract and execute_deauthorize_token_contract are nearly identical. Creating a generic deauthorization function will streamline the code.
Example refactor:
Create a generic deauthorization function:
fn execute_deauthorize_contract(
deps: DepsMut,
info: MessageInfo,
action: &str,
addr: AndrAddr,
) -> Result<Response, ContractError> {
let contract = ADOContract::default();
ensure!(
contract.is_contract_owner(deps.storage, info.sender.as_str())?,
ContractError::Unauthorized {}
);
let raw_addr = addr.get_raw_address(&deps.as_ref())?;
ADOContract::remove_permission(deps.storage, action, raw_addr.to_string())?;
Ok(Response::default().add_attributes(vec![
attr("action", format!("deauthorize_contract")),
attr("address", raw_addr),
]))
}Then refactor the original functions:
fn execute_deauthorize_cw20_contract(
deps: DepsMut,
info: MessageInfo,
cw20_address: AndrAddr,
) -> Result<Response, ContractError> {
execute_deauthorize_contract(deps, info, SEND_CW20_ACTION, cw20_address)
}
fn execute_deauthorize_token_contract(
deps: DepsMut,
info: MessageInfo,
token_address: AndrAddr,
) -> Result<Response, ContractError> {
execute_deauthorize_contract(deps, info, SEND_NFT_ACTION, token_address)
}Also applies to: 767-785
contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs
Outdated
Show resolved
Hide resolved
contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs
Show resolved
Hide resolved
contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs
Show resolved
Hide resolved
contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
packages/andromeda-non-fungible-tokens/src/auction.rs (1)
Line range hint
128-132: LGTM! Well-structured query with proper pagination.The AuthorizedAddresses query is well-designed with:
- Permission-based filtering
- Pagination support
- Flexible ordering options
Consider adding a doc comment to describe the expected behavior when filtering by different permission actions. This would help API consumers understand the query's capabilities.
/// Gets all of the authorized addresses for the auction + /// @param action Filter addresses by permission type (e.g., SendCw20, SendNft) + /// @param start_after Optional starting point for pagination + /// @param limit Optional maximum number of results + /// @param order_by Optional sorting preference #[returns(AuthorizedAddressesResponse)] AuthorizedAddresses {contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (1)
979-1117: Consider adding test for deauthorizing non-existent contractThe test coverage is good but could be enhanced by adding a test case for attempting to deauthorize a contract that was never authorized.
packages/std/src/common/denom.rs (1)
177-181: Add documentation toPermissionActionenum variants.Including documentation comments improves code maintainability and helps other developers understand the purpose of each variant.
You can add documentation as shown:
#[cw_serde] +/// Represents the possible permission actions. pub enum PermissionAction { + /// Permission to send CW20 tokens. SendCw20, + /// Permission to send NFTs. SendNft, }contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (1)
165-171: Proper handling of newAuthorizeContractandDeauthorizeContractmessagesThe
executefunction correctly matches the newExecuteMsg::AuthorizeContractandExecuteMsg::DeauthorizeContractvariants, passing the appropriate parameters toexecute_authorize_contractandexecute_deauthorize_contract. This aligns with the goal of generalizing authorization processes.Consider adding unit tests for new authorization functionalities
To ensure the robustness of the new authorization features, it's recommended to add unit tests for
AuthorizeContractandDeauthorizeContractexecution paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (4 hunks)
- contracts/non-fungible-tokens/andromeda-auction/src/mock.rs (2 hunks)
- contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs (6 hunks)
- contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (12 hunks)
- packages/andromeda-non-fungible-tokens/src/auction.rs (3 hunks)
- packages/andromeda-non-fungible-tokens/src/marketplace.rs (3 hunks)
- packages/std/src/common/denom.rs (2 hunks)
- packages/std/src/error.rs (1 hunks)
🔇 Additional comments (23)
packages/andromeda-non-fungible-tokens/src/marketplace.rs (4)
4-8: LGTM! Imports are well-organized and necessary.The new imports from
andromeda_std::commonare properly organized and necessary for the new authorization functionality.
22-22: LGTM! Implementation aligns with requirements.The change to support multiple CW20 addresses via
Vec<AndrAddr>aligns with the PR objectives from issue #585.
134-140: LGTM! Well-structured query implementation.The AuthorizedAddresses query is well-implemented with:
- Proper pagination support
- Optional ordering
- Type-safe action parameter using PermissionAction enum
48-58: Verify owner-only restrictions implementation.While the comments indicate these operations are "Restricted to owner", we should verify that the contract implementation properly enforces these restrictions.
✅ Verification successful
Based on the code review, I can now generate the final response since I have found clear evidence that the owner-only restrictions are properly implemented through the
execute_authorize_contractandexecute_deauthorize_contractfunctions which are imported fromandromeda_std::common::denom. The tests also confirm that unauthorized users cannot execute these operations.Owner-only restrictions are properly implemented.
The implementation correctly enforces owner-only access for both authorization operations:
- The
execute_authorize_contractandexecute_deauthorize_contractfunctions are imported from the standard library which handles owner validation- Test cases
test_execute_authorize_cw20_contractandtest_execute_deauthorize_cw20_contractverify that:
- Only the owner can authorize/deauthorize contracts
- Non-owner attempts result in
ContractError::Unauthorized🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the execute handler implementations to verify owner checks ast-grep --pattern 'fn execute_authorize_contract($$$) { $$$ }' ast-grep --pattern 'fn execute_deauthorize_contract($$$) { $$$ }'Length of output: 137
Script:
#!/bin/bash # Let's try a different approach to find the implementation # First, search for the execute function implementation that would handle these messages ast-grep --pattern 'fn execute($$$) { $$$ match msg { $$$ } $$$ }' # Also search for any owner validation patterns rg -A 5 "only_owner|assert_owner|owner_only"Length of output: 127
Script:
#!/bin/bash # Let's try searching for the implementation in the marketplace.rs file specifically cat packages/andromeda-non-fungible-tokens/src/marketplace.rs # Also search for any files that might contain the implementation fd marketplace.rs # And let's check for any test files that might show the authorization behavior rg -l "AuthorizeContract|DeauthorizeContract" --type rustLength of output: 4510
Script:
#!/bin/bash # Let's check the contract implementation and tests cat contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs cat contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rsLength of output: 71843
contracts/non-fungible-tokens/andromeda-auction/src/mock.rs (1)
13-13: LGTM! Import changes align with PR objectives.The addition of
PermissionActionimport is appropriate for the enhanced authorization functionality.packages/andromeda-non-fungible-tokens/src/auction.rs (2)
1-2: LGTM! Import changes align with new authorization features.The addition of
PermissionActionimport is consistent with the PR's objective to enhance authorization management for CW20 and CW721 addresses.
57-66: LGTM! Enhanced authorization controls with proper restrictions.The updated execute messages provide flexible authorization management with proper access controls and expiration support. The changes align well with the PR's objective of supporting multiple authorized addresses.
Let's verify the owner-only restrictions are properly implemented:
✅ Verification successful
Owner-only restrictions are properly implemented with access controls ✅
The implementation in
packages/std/src/common/denom.rsshows that bothexecute_authorize_contractandexecute_deauthorize_contractfunctions have proper owner-only access controls:
- Both functions use
ensure!(contract.is_contract_owner(...))check- Unauthorized access throws
ContractError::Unauthorized🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify owner access controls for authorize/deauthorize functions # Expected: Implementation should check for owner/admin privileges # Search for the execute handler implementations ast-grep --pattern 'fn execute_authorize_contract' -A 10 ast-grep --pattern 'fn execute_deauthorize_contract' -A 10Length of output: 119
Script:
#!/bin/bash # Let's try a different approach to find the implementation # First, let's find the file containing these functions rg -l "execute_authorize_contract|execute_deauthorize_contract" # Then search for owner/admin access control patterns rg "fn (execute_authorize_contract|execute_deauthorize_contract)" -A 10 # Also search for general owner/admin checks in the auction module rg "assert.*owner|ensure.*owner|require.*owner" packages/andromeda-non-fungible-tokens/src/auction.rsLength of output: 1852
contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (7)
107-115: LGTM: Init function properly handles multiple authorized addressesThe function signature change correctly implements the requirement to support multiple CW20 and NFT addresses during initialization.
202-241: LGTM: Comprehensive test coverage for CW20 authorizationThe test cases thoroughly verify:
- Unauthorized access prevention
- Successful authorization without expiration
- Successful authorization with expiration
- Correct permission storage
24-24:⚠️ Potential issueReplace
from_jsonwithfrom_binaryThe
from_jsonfunction is still being imported and used throughout the tests. As mentioned in past review comments, this should be replaced withfrom_binaryfromcosmwasm_std.
1142-1142:⚠️ Potential issueReplace
from_jsonwithfrom_binaryin query response handlingThe query response deserialization should use
from_binaryinstead offrom_json.Also applies to: 1156-1156
1204-1205:⚠️ Potential issueReplace
from_jsonwithfrom_binaryin query response handlingThe query response deserialization should use
from_binaryinstead offrom_json.
1253-1253:⚠️ Potential issueReplace
from_jsonwithfrom_binaryin query response handlingThe query response deserialization should use
from_binaryinstead offrom_json.
24-24: Verify all instances offrom_jsonusageLet's check for any remaining instances of
from_jsonusage in the codebase.✅ Verification successful
Verify remaining instances of
from_jsonusageBased on the search results,
from_jsonis still widely used throughout the codebase, primarily in:
- Test files (
tests.rs,mock.rs,mock_querier.rs) for deserializing query responses and test data- Contract execution handlers for parsing messages in hooks (e.g., Cw20HookMsg, Cw721HookMsg)
- Query handlers for deserializing requests
- IBC packet handling for deserializing packet data
The usage in the test file under review is consistent with the rest of the codebase, where
from_jsonis commonly used to deserialize query responses in tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances of from_json usage # Expected: No instances of from_json should be found after the fix # Search for from_json in imports echo "Checking imports..." rg 'from_json' -A 1 -B 1 # Search for from_json function calls echo "Checking function calls..." rg '::from_json|from_json\(' -A 1 -B 1Length of output: 135517
contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs (6)
6-11: Imports are correctly updated to support new featuresThe added imports for
AuthorizedAddressesResponse,ExecuteMsg,InstantiateMsg,QueryMsg, and other related structs are appropriate for the new authorization functionalities.
23-26: Denom module imports enhance permission handlingThe inclusion of
execute_authorize_contract,execute_deauthorize_contract,Asset,PermissionAction,SEND_CW20_ACTION, andSEND_NFT_ACTIONfrom thedenommodule correctly brings in the necessary components for contract authorization and asset management.
88-101: Consider refactoring to eliminate code duplication in initializationThe initialization logic for
authorized_cw20_addressesmirrors that ofauthorized_token_addresses. To improve maintainability and reduce repetition, consider abstracting this code into a helper function that handles authorization for both CW20 and CW721 addresses.
152-161: New execute handlers correctly delegate to authorization functionsThe added
ExecuteMsgvariants forAuthorizeContractandDeauthorizeContractcorrectly map to theexecute_authorize_contractandexecute_deauthorize_contractfunctions. This implementation provides the necessary functionality for dynamic permission management.
839-850: Query for authorized addresses implemented appropriatelyThe introduction of the
AuthorizedAddressesvariant inQueryMsgand its handling in thequeryfunction allows users to retrieve authorized addresses based on a specified action. The parametersstart_after,limit, andorder_bysupport pagination and ordering, enhancing query flexibility.
913-928: Usage ofPermissionActionenum ensures valid action parametersBy changing the
actionparameter to use thePermissionActionenum, thequery_authorized_addressesfunction now enforces that only valid actions are accepted. This enhancement improves type safety and prevents invalid or unexpected values from causing errors.contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (3)
15-21: Imports are correctly updated for new authorization functionalitiesThe added imports of
execute_authorize_contract,execute_deauthorize_contract,PermissionAction, andSEND_CW20_ACTIONare appropriate and necessary for handling the new authorization features.
1253-1259: Updated query function handlesAuthorizedAddresseswith theactionparameterThe
queryfunction has been appropriately updated to handle theAuthorizedAddressesquery variant with the newactionparameter, ensuring that queries can be made for specific permission actions.
1402-1409: Correct usage ofPermissionActioninquery_authorized_addressesIn the
query_authorized_addressesfunction, convertingPermissionActionto a string usingaction.as_str()ensures compatibility with thequery_permissioned_actorsmethod. This allows for accurate retrieval of authorized addresses based on the specified action.
joemonem
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
contracts/non-fungible-tokens/andromeda-auction/src/mock.rs (1)
Line range hint
26-279: Consider enhancing test coverage for the new authorization system.To ensure comprehensive testing of the new authorization system, consider adding:
- A mock function for testing deauthorization of contracts
- Mock functions for testing error cases (e.g., unauthorized addresses)
- Helper functions for testing multiple CW20 address scenarios
This would help ensure the robustness of the new authorization system.
contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (1)
1404-1411: Consider adding explicit error handling for invalid permission actions.While the implementation is correct, consider adding explicit error handling for invalid permission actions before passing them to the query system. This would provide clearer error messages to users when invalid actions are provided.
fn query_authorized_addresses( deps: Deps, action: PermissionAction, start_after: Option<String>, limit: Option<u32>, order_by: Option<OrderBy>, ) -> Result<AuthorizedAddressesResponse, ContractError> { + // Validate the action is supported + match action { + PermissionAction::SendCw20 | PermissionAction::SendNft => (), + _ => return Err(ContractError::InvalidPermissionAction { action: action.as_str().to_string() }), + } + let addresses = ADOContract::default().query_permissioned_actors( deps, action.as_str(), start_after, limit, order_by, )?; Ok(AuthorizedAddressesResponse { addresses }) }contracts/non-fungible-tokens/andromeda-auction/src/testing/tests.rs (4)
Line range hint
50-62: Add documentation for test helper functions.The initialization helper functions lack documentation explaining their purpose and parameters. Consider adding doc comments to improve test maintainability.
Add documentation like this:
+/// Initializes the auction contract with native token support +/// Returns the response from the instantiation fn init(deps: DepsMut) -> Response { // ... } +/// Initializes the auction contract with CW20 token support +/// Returns the response from the instantiation fn init_cw20(deps: DepsMut, _modules: Option<Vec<Module>>) -> Response { // ... }
Line range hint
447-556: Refactor long test into smaller, focused test cases.This test is quite long and tests multiple scenarios in a single function. Consider breaking it down into smaller, more focused tests and using constants for time values.
Split the test into separate test cases:
const ONE_SECOND: u64 = 1; const TWO_SECONDS: u64 = 2; const THREE_SECONDS: u64 = 3; #[test] fn test_execute_place_first_bid() { // Test first bid scenario } #[test] fn test_execute_place_higher_bid() { // Test higher bid scenario } #[test] fn test_execute_place_highest_bid() { // Test highest bid scenario }
Line range hint
1012-1034: Improve error handling test organization and constants.Error messages are hardcoded as strings and error handling tests could be organized more logically.
Consider using constants for error messages and grouping related error tests:
mod error_tests { const ERR_INVALID_FUNDS: &str = "Must provide at least 100 uusd to bid"; mod bid_validation { #[test] fn test_bid_below_min_price() { // Test using ERR_INVALID_FUNDS constant } // Group other bid validation error tests here } // Other error test groups }
Line range hint
556-557: Add missing edge case tests.Several edge cases appear to be missing or commented out. Consider adding tests for:
- Block height-based auctions
- Different expiration types
- Race conditions in bid placement
- Auction state transitions
Would you like me to help generate the missing test cases?
tests-integration/tests/auction_app.rs (4)
Line range hint
99-108: Refactor Repeated Wallet InitializationsThe wallet initializations for users like
owner,buyer_one,buyer_two, etc., are repeated across multiple test functions. Consider refactoring this code into a common setup function or using fixtures to reduce duplication. This will improve maintainability and make your tests cleaner.Apply this change to refactor the wallet setup:
- let mut router = mock_app(None); - let andr = MockAndromedaBuilder::new(&mut router, "admin") - .with_wallets(vec![ - ("owner", vec![]), - ("buyer_one", vec![coin(1000, "uandr")]), - ("buyer_two", vec![coin(1000, "uandr")]), - ("recipient_one", vec![]), - ("recipient_two", vec![]), - ]) - .with_contracts(vec![ - ("cw721", mock_andromeda_cw721()), - ("auction", mock_andromeda_auction()), - ("app-contract", mock_andromeda_app()), - ("splitter", mock_andromeda_splitter()), - ]) - .build(&mut router); - let owner = andr.get_wallet("owner"); - let buyer_one = andr.get_wallet("buyer_one"); - let buyer_two = andr.get_wallet("buyer_two"); - let recipient_one = andr.get_wallet("recipient_one"); - let recipient_two = andr.get_wallet("recipient_two"); + // Initialize wallets and contracts using a common setup function + let (mut router, andr, owner, buyer_one, buyer_two, recipient_one, recipient_two) = setup_test_env();
Line range hint
179-181: Handle Potential Errors inexecute_claim_auctionIn lines 179-181, when calling
execute_claim_auction, there is no error handling if the auction claim fails. Consider adding assertions or error checks to ensure that the auction claim has been executed successfully. This will help in identifying issues during the claim process in tests.
532-535: Improve Readability of Auction InitializationThe parameters in the
mock_auction_instantiate_msgcall at lines 532-535 span multiple lines and include nested functions. For better readability, consider assigning complex parameters to variables before the function call. This makes the code easier to read and maintain.Refactor the code as follows:
+ let authorized_cw20_addresses = Some(vec![AndrAddr::from_string(format!( + "./{}", + cw20_component.name + ))]); let auction_init_msg = mock_auction_instantiate_msg( andr.kernel.addr().to_string(), None, Some(vec![AndrAddr::from_string(format!( "./{}", cw721_component.name ))]), - Some(vec![AndrAddr::from_string(format!( - "./{}", - cw20_component.name - ))]), + authorized_cw20_addresses, );
Line range hint
942-945: Avoid Hardcoding Time ValuesIn lines 942-945, you're manually converting timestamps using
MILLISECONDS_TO_NANOSECONDS_RATIO. Consider using Rust's time manipulation libraries or utility functions to handle time conversions and manipulations. This will reduce the risk of errors due to incorrect calculations and improve code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (5 hunks)
- contracts/non-fungible-tokens/andromeda-auction/src/mock.rs (3 hunks)
- contracts/non-fungible-tokens/andromeda-auction/src/testing/tests.rs (2 hunks)
- packages/andromeda-non-fungible-tokens/src/auction.rs (4 hunks)
- tests-integration/tests/auction_app.rs (1 hunks)
🔇 Additional comments (12)
contracts/non-fungible-tokens/andromeda-auction/src/mock.rs (3)
13-13: LGTM: Import addition is appropriate.The addition of
PermissionActionimport aligns with the new authorization system implementation.
171-171: LGTM: Parameter addition supports multiple CW20 addresses.The addition of
authorized_cw20_addressesasOption<Vec<AndrAddr>>correctly implements the requirement for supporting multiple CW20 addresses during instantiation.Also applies to: 177-177
212-214: Referencing existing review comment.The previous review comment about enhancing the mock function's flexibility is still valid and applicable to these changes.
packages/andromeda-non-fungible-tokens/src/auction.rs (4)
2-2: LGTM: Import addition aligns with new permission system.The addition of
PermissionActionimport supports the new permission-based contract authorization system.
17-17: LGTM: Field change enables multiple CW20 addresses support.The modification from
authorized_cw20_addresstoauthorized_cw20_addresseswithOption<Vec<AndrAddr>>type correctly implements the requirement for supporting multiple CW20 addresses, maintaining consistency with theauthorized_token_addressesfield.
Line range hint
128-132: LGTM: Query enhancement allows permission-specific address listing.The addition of
action: PermissionActionto theAuthorizedAddressesquery enables filtering addresses by permission type, maintaining consistency with the new authorization system while preserving pagination functionality.
57-66: LGTM: Enhanced contract authorization system with flexible permissions.The updated message structure with
PermissionActionprovides a more flexible and reusable authorization system. The addition of optional expiration for authorizations is a good security practice.Let's verify the consistency of PermissionAction usage across the codebase:
✅ Verification successful
PermissionAction implementation is consistent and well-structured across the codebase
The verification confirms that:
PermissionActionis properly defined in the standard library (packages/std/src/common/denom.rs) with two actions:SendCw20andSendNft- The authorization implementations in both marketplace and auction contracts correctly use this enum
- The base implementation in the standard library ensures proper owner-only authorization checks
- The expiration functionality is consistently implemented across all uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of PermissionAction in contract authorization # Expected: All contract authorization implementations should use PermissionAction # Check for AuthorizeContract implementations ast-grep --pattern 'fn execute_authorize_contract($$$) { $$$ }' # Check for DeauthorizeContract implementations ast-grep --pattern 'fn execute_deauthorize_contract($$$) { $$$ }'Length of output: 137
Script:
#!/bin/bash # Let's try a different approach to find PermissionAction usage and authorization implementations # Find all Rust files that contain PermissionAction rg -l "PermissionAction" --type rust # Search for AuthorizeContract and DeauthorizeContract in the codebase rg "(?:Authorize|Deauthorize)Contract" --type rust -A 5 # Look for struct or enum definitions containing PermissionAction ast-grep --pattern 'enum $_ { $$$ PermissionAction $$$ }' ast-grep --pattern 'struct $_ { $$$ action: PermissionAction, $$$ }'Length of output: 12416
Script:
#!/bin/bash # Let's check the PermissionAction definition and its implementation in the std package # Check PermissionAction definition rg -A 10 "enum PermissionAction" --type rust # Check for the implementation of authorization functions in ado_contract rg -A 10 "fn (execute_authorize_contract|execute_deauthorize_contract)" --type rustLength of output: 2205
contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (2)
82-92: LGTM: CW20 authorization implementation is consistent and secure.The implementation follows the same secure pattern as NFT authorization, properly validating addresses and setting appropriate permissions.
167-173: LGTM: Authorization implementation aligns with PR objectives.The implementation uses standardized authorization functions from andromeda_std, providing a consistent approach for both CW20 and NFT contract authorization.
tests-integration/tests/auction_app.rs (3)
532-535: Ensure Consistent Usage ofauthorized_cw20_addressesIn lines 532-535, the
mock_auction_instantiate_msgfunction is called withauthorized_cw20_addressesset to a vector containing one CW20 address. Ensure that all test cases and function calls that rely on this variable are updated to handle multiple CW20 addresses consistently. This will prevent any unexpected behavior when the auction interacts with authorized CW20 tokens.
Line range hint
680-688: Validate Auction Parameters with Unrestricted CW20 TokensIn lines 680-688, you're creating a new auction with a different CW20 token without permissions. Ensure that the auction contract correctly handles CW20 tokens that are not explicitly authorized when
authorized_cw20_addressesisNone. This test should verify that the auction accepts bids with any CW20 token as intended.
Line range hint
769-777: Check Balance Updates After Auction CompletionIn lines 769-777, after the auction ends, you check the balances of the owner and bidders. Ensure that the balance updates reflect the correct amounts, especially considering any fees or rates that might apply. Double-check that the owner's balance increases appropriately and that the bidders' balances decrease based on their bids.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
CHANGELOG.md (1)
33-33: Consider enhancing the changelog entry with more specific details.While the current entry covers the main features, consider expanding it to explicitly mention:
- Support for multiple CW20 addresses
- New query functionality for authorized addresses
- Version bump from 2.1.4 to 2.2.4
Suggested enhancement:
- Auction & Marketplace: Added Multiple Authorized Addresses During Init, Add and Remove Authorized Addresses [(#589)](https://github.com/andromedaprotocol/andromeda-core/pull/589) + Auction & Marketplace: Added support for multiple CW20 authorized addresses, including initialization, authorization, deauthorization, and query functionality. Version bump to 2.2.4 [(#589)](https://github.com/andromedaprotocol/andromeda-core/pull/589)packages/std/src/common/denom.rs (2)
127-146: Add documentation for the public function.The
authorize_addressesfunction lacks documentation. Consider adding a doc comment explaining:
- Purpose of the function
- Parameters description
- Return value description
- Example usage
Example documentation:
/// Authorizes multiple addresses for a specific action. /// /// # Arguments /// * `deps` - Mutable dependencies /// * `action` - The permission action to authorize /// * `addresses` - List of addresses to authorize /// /// # Returns /// * `Ok(())` on success /// * `ContractError` on failure
230-258: Add tests for authorize_addresses function.The new
authorize_addressesfunction lacks test coverage. Consider adding tests for:
- Successful authorization of multiple addresses
- Empty address list handling
- Invalid address handling
- Permission validation failures
Example test cases:
#[test] fn test_authorize_addresses() { let mut deps = mock_dependencies(); // Test successful authorization let addresses = vec![ AndrAddr::from_string("addr1"), AndrAddr::from_string("addr2"), ]; let result = authorize_addresses(&mut deps.as_mut(), SEND_CW20_ACTION, addresses); assert!(result.is_ok()); // Test empty address list let result = authorize_addresses(&mut deps.as_mut(), SEND_CW20_ACTION, vec![]); assert!(result.is_ok()); // Test invalid action let result = authorize_addresses(&mut deps.as_mut(), "INVALID", vec![AndrAddr::from_string("addr1")]); assert!(matches!(result, Err(ContractError::InvalidAction { .. }))); }contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs (1)
883-898: Consider adding error handling documentation.While the implementation is correct, consider documenting the possible error conditions that could be returned from
query_permissioned_actors. This would help API consumers better handle error cases.Add documentation like:
/// Query authorized addresses for a specific action /// /// # Errors /// /// * `ContractError::Unauthorized` - If the caller lacks permission to query /// * `ContractError::InvalidAction` - If the action is not supported /// * `ContractError::StorageError` - If there's an error reading from storagecontracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (1)
Test coverage appears comprehensive with one caveat.
The test suite in
andromeda-marketplace/src/testing/tests.rsis extensive and covers the new functionality for CW20 address management. However, there is one TODO comment that needs attention:
// TODO having both tax and royalty is currently unsupportedThis indicates a potential gap in test coverage for scenarios involving both tax and royalty calculations.
🔗 Analysis chain
Line range hint
1-1256: Verify test coverage metrics.While the tests appear comprehensive, it would be beneficial to verify the test coverage metrics to ensure all code paths are adequately tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for the marketplace contract # Note: This assumes cargo-tarpaulin is available in the environment # Search for any test files echo "Searching for test files..." fd -e rs "test" "src" # Look for any TODOs or unimplemented tests echo -e "\nChecking for TODO comments in tests..." rg -i "todo|fixme|xxx" "src/testing"Length of output: 357
Script:
#!/bin/bash # Let's try searching from the repository root echo "Searching for test files..." fd -t f -e rs "tests?" . echo -e "\nChecking for TODO comments in tests..." rg -i "todo|fixme|xxx" . --type rustLength of output: 5583
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
- CHANGELOG.md (1 hunks)
- contracts/non-fungible-tokens/andromeda-auction/Cargo.toml (1 hunks)
- contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (6 hunks)
- contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs (6 hunks)
- contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (12 hunks)
- packages/andromeda-non-fungible-tokens/src/auction.rs (4 hunks)
- packages/andromeda-non-fungible-tokens/src/marketplace.rs (4 hunks)
- packages/std/src/common/denom.rs (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/non-fungible-tokens/andromeda-auction/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/andromeda-non-fungible-tokens/src/auction.rs
- packages/andromeda-non-fungible-tokens/src/marketplace.rs
🔇 Additional comments (10)
packages/std/src/common/denom.rs (1)
3-16: LGTM: Clean imports and well-structured response type.The new imports and
AuthorizedAddressesResponsestruct align well with the PR objectives for managing multiple authorized addresses.Also applies to: 122-125
contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs (4)
17-20: LGTM: Imports align with multiple address authorization feature.The new imports from
andromeda_std::common::denomprovide the necessary functionality for managing multiple authorized addresses.
122-130: LGTM: Authorization handlers properly implemented.The new execute handlers for authorizing and deauthorizing contracts are well-integrated and consistent with the existing codebase patterns.
809-820: LGTM: Query handler properly implemented with pagination support.The AuthorizedAddresses query handler is well-implemented with proper pagination support through start_after and limit parameters.
70-71: Verify CW20 address validation.While the authorization logic is correct, consider verifying that the provided CW20 addresses are valid contract addresses to prevent authorization of non-existent or invalid contracts.
✅ Verification successful
Based on the search results, I can now provide a final response:
CW20 addresses are already validated through multiple layers
The CW20 address validation is handled through several mechanisms:
- The
get_verified_assetfunction is used to validate assets including CW20 tokens- The
AndrAddrstruct provides validation through itsvalidateandget_raw_addressmethods- The Cosmos SDK's
addr_validateis used to verify the CW20 contract addresses- Additional validation occurs through the ADO contract's permission system
The marketplace contract already implements proper validation through:
- Address validation during authorization
- Asset verification before transactions
- Permission checks for CW20 operations
- Integration with the ADO contract's validation system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the CW20 addresses are valid contract addresses # Note: This script helps identify potential validation needs # Search for CW20 contract validation patterns in the codebase rg -A 3 "fn validate_cw20|check_cw20|verify_cw20" # Search for any existing CW20 address validation ast-grep --pattern 'if let Some($_) = $addresses { for $addr in $_ { $validate } }'Length of output: 145
Script:
#!/bin/bash # Let's try a broader search for address validation patterns and CW20-related code # Search for CW20-related code and validation patterns rg -A 5 "cw20|Cw20|validate.*address|address.*validate" contracts/ # Look for any address validation in the authorize_addresses function rg -A 10 "fn authorize_addresses" contracts/ # Check if there's any address validation in dependencies or imports rg -A 3 "use.*cw20|use.*validate" contracts/Length of output: 462559
Script:
#!/bin/bash # Let's search for address validation patterns in the codebase, particularly around CW20 authorization # Search for address validation in authorize_addresses function rg -A 5 'fn authorize_addresses|pub fn authorize_addresses' contracts/ # Search for any CW20 address validation patterns rg -A 5 'validate.*cw20|cw20.*validate|verify.*cw20|check.*cw20' contracts/ # Search for any address validation in the marketplace contract rg -A 5 'validate|verify|check' contracts/non-fungible-tokens/andromeda-marketplace/src/Length of output: 32614
Script:
#!/bin/bash # Let's look for address validation in the marketplace contract and related functions # Search for authorize_addresses function implementation rg -A 10 'fn authorize_addresses' contracts/non-fungible-tokens/andromeda-marketplace/src/ # Search for address validation in ADOContract or andromeda_std rg -A 5 'fn addr_validate|validate_address|validate_addresses' contracts/ # Look for any CW20 validation in andromeda_std rg -A 5 'validate.*cw20|verify.*cw20|check.*cw20' packages/Length of output: 1127
Script:
#!/bin/bash # Let's look for the implementation of get_verified_asset and related validation # Search for get_verified_asset implementation rg -A 10 'fn get_verified_asset|impl.*get_verified_asset' contracts/ # Look for address validation in andromeda_std common modules rg -A 5 'validate|verify|check' packages/std/src/common/ # Look for CW20 validation in andromeda_std amp module rg -A 5 'validate|verify|check' packages/std/src/amp/Length of output: 13522
contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (4)
107-108: LGTM! Comprehensive test coverage for multiple CW20 addresses initialization.The test thoroughly verifies the new functionality to support multiple authorized CW20 addresses during initialization, including permission checks and negative testing.
Also applies to: 203-242
980-1118: LGTM! Thorough test coverage for CW20 contract authorization.The tests comprehensively cover all aspects of CW20 contract authorization including:
- Authorization with and without expiration
- Permission verification
- Unauthorized attempts
- Deauthorization flow
1142-1143: Fixfrom_jsonusage in query response deserialization.As noted in previous reviews,
from_jsonshould be replaced withfrom_binaryfor query response deserialization.Also applies to: 1156-1157, 1205-1206, 1254-1255
1163-1256: LGTM! Consistent test coverage for NFT contract authorization.The tests maintain consistency with the CW20 authorization pattern and provide thorough coverage of NFT contract authorization functionality.
contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (1)
68-72: LGTM: Authorization implementation is clean and consistent.The implementation properly handles both NFT and CW20 address authorization during contract instantiation, aligning with the PR objectives to support multiple authorized addresses.
(cherry picked from commit c7f2fb7) # Conflicts: # CHANGELOG.md
Co-authored-by: Joe Monem <[email protected]>
Motivation
Closes #585 , #586 and #587
Implementation
Implement ability to add or remove authorized cw20 and cw721 addresses, allow multiple cw20 addresses to be set during instantiation for marketplace and auction, query function for those addresses.
Testing
Unit tests
Version Changes
Marketplace
"2.1.4"->"2.2.4"Auction
"2.1.4"->"2.2.4"Summary by CodeRabbit
Release Notes
New Features
PermissionActionenum for managing contract permissions.Bug Fixes
Tests