feat: Marketplace & Auction Adjustments #589
Conversation
…d remove authorized cw20 contracts functions, unit tests
|
Warning Rate limit exceeded@joemonem has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several changes to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
| ExecuteMsg::DeauthorizeCw20Contract { addr } => { | ||
| execute_deauthorize_cw20_contract(ctx.deps, ctx.info, addr) | ||
| } | ||
| ExecuteMsg::AuthorizeTokenContract { addr, expiration } => { |
There was a problem hiding this comment.
We should keep naming consistent. Maybe AuthorizeCw721Contract?
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.
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