contracts/data-storage/andromeda-boolean/src/contract.rs (1)

Line range hint 1-59: Consider adding documentation for the permission timing behavior.

Since this contract implements permission-based access control with timing constraints, it would be beneficial to add documentation explaining:

• The behavior when start_time is not reached
• The interaction between start_time and end_time
• The default behavior when both timing parameters are None

Add rustdoc comments above the instantiate function:

/// Instantiates the boolean storage contract with optional permission timing constraints.
/// 
/// # Arguments
/// 
/// * `restriction` - Determines if the storage is private. If private, only whitelisted
///   addresses can modify the value.
/// * For private storage, permissions can have optional start and end times:
///   - Before start_time: Access follows the opposite of the `strict` setting
///   - Between start_time and end_time: Normal permission rules apply
///   - After end_time: Permission expires

contracts/os/andromeda-ibc-registry/src/contract.rs (1)

59-61: Consider adding start/end time validation.

As noted in the PR objectives, there's currently no validation to prevent setting a start time that occurs after the end time. Consider adding this validation in the permission setting logic.

Would you like me to propose an implementation for the time validation logic?

packages/std/src/common/denom.rs (1)

162-163: Consider adding start/end time validation.

The permission construction correctly handles the optional expiration time. However, as noted in the PR objectives, there's currently no validation to prevent setting a start time that occurs after the end time. Consider adding this validation here.

Here's a suggested implementation:

 let permission = expiration.map_or(
     Permission::Local(LocalPermission::whitelisted(None, None)),
-    |expiration| Permission::Local(LocalPermission::whitelisted(None, Some(expiration))),
+    |expiration| {
+        // When start time support is added, add validation here
+        // if let (Some(start), Some(end)) = (start_time, expiration) {
+        //     ensure!(
+        //         start <= end,
+        //         ContractError::InvalidTimeRange {
+        //             msg: "Start time must be before or equal to end time".to_string()
+        //         }
+        //     )?;
+        // }
+        Permission::Local(LocalPermission::whitelisted(None, Some(expiration)))
+    }
 );

contracts/fungible-tokens/andromeda-cw20/src/testing/tests.rs (2)

115-115: LGTM on permission constructor changes, but test coverage needs expansion.

The updated constructor calls for LocalPermission::blacklisted(None, None) and LocalPermission::whitelisted(None, None) align with the new API that supports start time. However, the current test only verifies basic permission functionality.

Consider adding the following test cases to validate the new start time feature:

#[test]
fn test_permission_with_start_time() {
    let mut deps = mock_dependencies_custom(&[]);
    let start = Expiry::AtTime(Timestamp::from_seconds(mock_env().block.time.seconds() + 100));
    
    // Test future start time
    let permission = Permission::Local(LocalPermission::blacklisted(None, Some(start)));
    // ... verify is_permissioned returns true before start time
    
    // Test both start and end times
    let end = Expiry::AtTime(Timestamp::from_seconds(mock_env().block.time.seconds() + 200));
    let permission = Permission::Local(LocalPermission::whitelisted(Some(end), Some(start)));
    // ... verify permission behavior before start, between start/end, and after end
}

Also applies to: 128-128

---

Line range hint 115-128: Add validation for start/end time ordering.

The PR notes there's currently no validation to prevent setting a start time that occurs after the end time. This could lead to unexpected behavior.

Consider adding validation tests and implementing the validation:

#[test]
fn test_invalid_permission_times() {
    let mut deps = mock_dependencies_custom(&[]);
    let earlier = Expiry::AtTime(Timestamp::from_seconds(mock_env().block.time.seconds() + 100));
    let later = Expiry::AtTime(Timestamp::from_seconds(mock_env().block.time.seconds() + 200));
    
    // Should fail when start time is after end time
    let result = Permission::Local(LocalPermission::whitelisted(Some(earlier), Some(later)));
    assert!(matches!(result, Err(ContractError::InvalidStartTime { .. })));
    
    // Should succeed when times are properly ordered
    let result = Permission::Local(LocalPermission::whitelisted(Some(later), Some(earlier)));
    assert!(result.is_ok());
}

contracts/modules/andromeda-address-list/src/testing/tests.rs (1)

Line range hint 1-300: Add test coverage for start time functionality.

The test file needs additional test cases to cover the new start time feature:

1. Test start time behavior for whitelisted permissions
2. Test start time behavior for blacklisted permissions
3. Test start time behavior for limited permissions
4. Test interaction between start time and end time
5. Test edge cases with invalid start/end time combinations

Here's a suggested test case structure:

#[test]
fn test_permission_start_time() {
    let mut deps = mock_dependencies_custom(&[]);
    let env = mock_env();
    let info = mock_info("creator", &[]);
    
    // Setup timestamp for testing
    let now = env.block.time;
    let future = now.plus_seconds(100);
    let past = now.minus_seconds(100);
    
    // Test cases to add:
    
    // 1. Whitelisted permission with future start time
    let permission = LocalPermission::whitelisted(Some(future), None);
    // Verify is_permissioned returns false before start time
    // Verify is_permissioned returns true after start time
    
    // 2. Blacklisted permission with future start time
    let permission = LocalPermission::blacklisted(Some(future), None);
    // Verify is_permissioned returns true before start time
    // Verify is_permissioned returns false after start time
    
    // 3. Limited permission with future start time
    let permission = LocalPermission::limited(Some(future), None, 5);
    // Verify is_permissioned behavior before and after start time
    
    // 4. Test start time after end time
    let permission = LocalPermission::whitelisted(Some(future), Some(past));
    // Verify behavior or validate that this is prevented
}

Would you like me to create a GitHub issue to track this test coverage task?

contracts/fungible-tokens/andromeda-merkle-airdrop/src/contract.rs (1)

72-72: Consider adding validation for start/end time relationship

The change correctly implements start time support for permissions. However, as noted in the PR description, there's no validation to prevent setting a start time that occurs after the end time, which could lead to unexpected behavior.

Consider adding validation in the LocalPermission::whitelisted constructor to ensure that when both times are provided, start_time ≤ end_time.

contracts/non-fungible-tokens/andromeda-crowdfund/src/contract.rs (2)

Line range hint 385-394: Add validation for start time being in the future

While the code validates that start time is before end time, it should also validate that the start time is in the future when provided. This would prevent accidentally setting a start time in the past.

Apply this diff to add the validation:

     ensure!(
         start_time_milliseconds.map_or(end_time_milliseconds > current_time, |start| start
-            <= end_time_milliseconds),
+            <= end_time_milliseconds && start > current_time),
         if start_time_milliseconds.is_some() {
             ContractError::StartTimeAfterEndTime {}
         } else {
             ContractError::InvalidExpiration {}
         }
     );

---

Line range hint 385-394: Encapsulate time validation in the Duration struct

Consider moving the time validation logic into the Duration struct to ensure consistent validation across all usage points. This would also make it easier to add validation preventing start time from being after end time, as mentioned in the PR objectives.

Consider implementing validation methods in the Duration struct:

impl Duration {
    pub fn validate(&self, current_time: Milliseconds) -> Result<(), ContractError> {
        if let Some(start_time) = self.start_time {
            ensure!(
                start_time > current_time,
                ContractError::InvalidStartTime {}
            );
            ensure!(
                start_time <= self.end_time,
                ContractError::StartTimeAfterEndTime {}
            );
        }
        ensure!(
            self.end_time > current_time,
            ContractError::InvalidExpiration {}
        );
        Ok(())
    }
}

contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (1)

Line range hint 312-319: Consider adding start/end time validation for permissions.

The PR objectives mention that there's no validation to prevent setting a start time that occurs after the end time. Consider adding this validation when setting permissions.

Would you like me to propose an implementation for the validation logic?

Also applies to: 426-433

packages/std/src/ado_base/permissioning.rs (2)

69-72: Ensure the default Whitelisted permission aligns with expected security policies

Setting the default permission to Whitelisted with start and expiration as None effectively grants indefinite permission starting immediately. This might not align with the desired security posture.

Consider whether the default should be more restrictive, or make it explicit that the default is an open permission. Alternatively, you could require explicit start and expiration times to avoid unintended permission grants.

---

Line range hint 95-134: Simplify and refactor the is_permissioned method for better readability

The is_permissioned method contains repeated code blocks for checking start times. This repetition can be reduced to improve maintainability.

Refactor the is_permissioned method to extract common logic:

pub fn is_permissioned(&self, env: &Env, strict: bool) -> bool {
    let current_time = env.block.time;
    let started = |start: &Option<Expiry>| {
        start
            .as_ref()
            .map_or(true, |s| s.get_time(&env.block).is_expired(&env.block))
    };
    let not_expired = |expiration: &Option<Expiry>| {
        expiration
            .as_ref()
            .map_or(true, |e| !e.get_time(&env.block).is_expired(&env.block))
    };

    match self {
        Self::Blacklisted { start, expiration } => {
            if !started(start) {
                return true;
            }
            if !not_expired(expiration) {
                return !strict;
            }
            false
        }
        Self::Limited { start, expiration, uses } => {
            if !started(start) {
                return true;
            }
            if !not_expired(expiration) || *uses == 0 {
                return !strict;
            }
            true
        }
        Self::Whitelisted { start, expiration } => {
            if !started(start) {
                return !strict;
            }
            if !not_expired(expiration) {
                return !strict;
            }
            true
        }
    }
}

tests-integration/tests/validator_staking.rs (1)

279-279: Consider adding tests for start_time and end_time scenarios.

To ensure the robustness of the permissioning system, it would be beneficial to add test cases that cover scenarios where start_time and end_time are utilized. This includes edge cases such as when the start_time is after the end_time, or when the current time is before the start_time.

tests-integration/tests/auction_app.rs (1)

615-618: Consider adding tests with specific start_time and end_time for permissions

Currently, permissions are set with None for both start_time and end_time. To fully validate the new permission timing features, consider adding tests where these parameters are set to specific values to ensure that permissions are correctly enforced over time.

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Verify the LocalPermission::whitelisted method implementation
# Expected: Method should accept start_time and end_time parameters of type Option<Expiry>

# Search for the whitelisted method definition
ast-grep --pattern 'impl LocalPermission {
  $$$
  fn whitelisted($$$) {
    $$$
  }
  $$$
}'

# Search for any related tests
rg -A 5 "test.*permission.*start_time"

#!/bin/bash
# Search for LocalPermission definition and implementation in the codebase
rg -A 10 "enum LocalPermission|struct LocalPermission" 

# Search specifically in the std package where we found the test
cat packages/std/src/ado_contract/permissioning.rs

#!/bin/bash
# Search for LocalPermission enum definition
rg -A 10 "enum LocalPermission" packages/std/src/ado_base/permissioning.rs

#!/bin/bash
# Get the complete LocalPermission enum definition
cat packages/std/src/ado_base/permissioning.rs

#!/bin/bash
# Description: Verify the permission structure and its usage across the codebase

# Check the LocalPermission struct definition
rg -A 5 "struct LocalPermission" 

# Check all usages of whitelisted to ensure consistent parameter order
rg "whitelisted\(.*None.*None"

# Look for any validation of start_time vs end_time
rg "start_time.*end_time"

#!/bin/bash
# Let's check the LocalPermission struct definition and implementation
# since it wasn't found in the first search

# Search for LocalPermission in andromeda-std crate
fd -t f "lib.rs|mod.rs" -x rg -A 10 "LocalPermission" {}

# Search for whitelisted function implementation
ast-grep --pattern 'fn whitelisted($$$) { $$$ }'

# Check for any time validation in the permission module
fd "permission" -x rg -A 5 "validate.*time" {}

#!/bin/bash
# Try a different approach to find LocalPermission definition and implementation

# Search for enum or struct definitions containing LocalPermission
rg -A 15 "(enum|struct) .*Permission"

# Look for the implementation block of LocalPermission
rg -A 10 "impl.*LocalPermission"

# Search for whitelisted method with a broader pattern
rg -A 5 "whitelisted.*None.*None"

#!/bin/bash
# Description: Check for any remaining old-style permission constructors
# that haven't been updated to the new format

# Search for old-style permission constructors
rg "LocalPermission::Whitelisted\(" --type rust

# Search for new-style permission constructors to compare
rg "LocalPermission::whitelisted\(" --type rust

#!/bin/bash
# Description: Find test files and locate the test_permission_start_time test

# Find all test files in the repository
echo "=== Test files ==="
fd -e rs "test" -t f

# Search for test_permission_start_time
echo -e "\n=== Searching for test_permission_start_time ==="
rg -l "test_permission_start_time"

# Search for LocalPermission usage in tests
echo -e "\n=== Test files using LocalPermission ==="
rg -l "LocalPermission::" -- "*test*.rs"

#!/bin/bash
# Let's check the content of the permissioning.rs file and search for LocalPermission usage in tests with correct path patterns

# Check the content of permissioning.rs where test_permission_start_time was found
echo "=== Content of permissioning.rs ==="
cat packages/std/src/ado_contract/permissioning.rs

# Search for LocalPermission usage in tests with correct path pattern
echo -e "\n=== Test files using LocalPermission ==="
rg -l "LocalPermission::" "**/*test*.rs"

# Search for test_permission_start_time in test files
echo -e "\n=== Test files containing test_permission_start_time ==="
rg -l "test_permission_start_time" "**/*test*.rs"

#!/bin/bash
# Let's try a different approach to search for the test and LocalPermission usage

# Search for test_permission_start_time in all files
echo "=== Files containing test_permission_start_time ==="
rg "test_permission_start_time"

# Search for LocalPermission usage in test files
echo -e "\n=== Test files using LocalPermission ==="
rg "LocalPermission::" -t rust

# List all test files that import LocalPermission
echo -e "\n=== Test files importing LocalPermission ==="
rg "use.*LocalPermission" -t rust

ensure!(
    end_expiration > start_expiration,
    ContractError::StartTimeAfterEndTime {}
);

#!/bin/bash
# Search for any blacklisted permission usage that might need updating
rg -A 2 'LocalPermission::Blacklisted' || rg -A 2 'LocalPermission::blacklisted'

#!/bin/bash
# Check the LocalPermission type definition and its constructors
ast-grep --pattern 'enum LocalPermission {
  $$$
}'

# Also check for any whitelisted constructor implementation
ast-grep --pattern 'impl LocalPermission {
  $$$
}'

#!/bin/bash
# Description: Find calls to permission factory methods with `start` and `expiration` as `None`.

rg 'LocalPermission::(blacklisted|whitelisted|limited)\(None,\s*None' -A 2

#!/bin/bash
# Find the implementation that checks these permissions
ast-grep --pattern 'impl LocalPermission {
  $$$
  pub fn check($$$) {
    $$$
  }
  $$$
}'

#!/bin/bash
# Find the implementation that checks these permissions using ripgrep
rg "impl.*LocalPermission" -A 20

#!/bin/bash
# Get the complete implementation of is_permissioned
rg "pub fn is_permissioned" -A 30

CHANGELOG.md (1)

78-78: Enhance the changelog entry to better reflect the scope of changes.

While the entry follows the correct format, consider expanding it to better communicate the impact and limitations of this change. Here's a suggested revision:

-Permissions now have an optional start time [(#668)](https://github.com/andromedaprotocol/andromeda-core/pull/668)
+Enhanced permission system with optional start time, affecting how `is_permissioned` evaluates blacklisted, whitelisted, and limited permissions based on start time. Note: No validation between start and end times [(#668)](https://github.com/andromedaprotocol/andromeda-core/pull/668)

Configuration used: CodeRabbit UI
Review profile: CHILL

tests-integration/tests/marketplace_app.rs (4)

Line range hint 600-627: Refactor repetitive permission settings to improve maintainability

The code between lines 600-627 and 903-939 repeats the execute_actor_permission calls with LocalPermission::whitelisted(None, None) for different actors. This repetition can lead to code that is harder to maintain and prone to errors if changes are needed in the future.

Consider creating a helper function or loop to set permissions for multiple actors. This will reduce code duplication and make future updates easier.

Example refactor:

// Define a list of actors to whitelist
let actors_to_whitelist = vec![
    cw721.addr().clone(),
    cw20.addr().clone(),
    buyer.clone(),
    owner.clone(),
];

// Iterate over the actors and set permissions
for actor in actors_to_whitelist {
    address_list
        .execute_actor_permission(
            &mut router,
            owner.clone(),
            actor,
            LocalPermission::whitelisted(None, None),
        )
        .unwrap();
}

Also applies to: 903-939

---

Line range hint 600-627: Add validation for start_time and end_time in permissions

Currently, there is no validation to prevent users from setting a start_time that occurs after the end_time in LocalPermission::whitelisted(None, None). This could lead to unexpected behavior in permission handling.

Ensure that when both start_time and end_time are provided, the code validates that start_time is before end_time. This can prevent logical errors and improve the robustness of the permission system.

Example validation logic:

impl LocalPermission {
    pub fn whitelisted(start_time: Option<Timestamp>, end_time: Option<Timestamp>) -> Self {
        if let (Some(start), Some(end)) = (start_time, end_time) {
            if start >= end {
                panic!("start_time must be before end_time");
            }
        }
        // Existing implementation
    }
}

---

Line range hint 600-627: Consider adding unit tests for permission time boundaries

With the introduction of start_time and end_time, it's important to test how permissions behave at these boundaries. There are currently no tests covering scenarios where permissions are not yet active or have expired.

Consider adding unit tests that:

• Verify permissions are not granted before start_time.
• Verify permissions are revoked after end_time.
• Test cases where start_time is equal to end_time.

This will ensure that time-based permissions work as expected.

---

Line range hint 903-939: Consider combining similar test scenarios to reduce redundancy

The tests test_marketplace_app_cw20_restricted and test_marketplace_app_cw20_unrestricted share similar setup code. Refactoring common setup steps into helper functions can make the tests cleaner and reduce duplication.

Extract shared setup code into helper functions. This will make your tests more maintainable and easier to understand.

Example:

fn setup_marketplace_app_cw20_test(
    router: &mut MockApp,
    andr: &MockAndromedaBuilder,
    owner: &Addr,
    buyer: &Addr,
    rates_receiver: &Addr,
) -> (MockCW721, MockMarketplace, MockCW20, MockRates) {
    // Common setup code here...

    (cw721, marketplace, cw20, rates)
}

Then, in your tests, you can call this function to perform the setup.

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Find calls to LocalPermission::whitelisted with incorrect arguments

# Expected: All calls should include two arguments
rg 'LocalPermission::whitelisted\([^,)]*\)'

#!/bin/bash
# Let's try a broader search to find all instances of LocalPermission::whitelisted
rg "LocalPermission::whitelisted" -A 2

#!/bin/bash
# Let's also check the struct/enum definition to understand the expected parameters
ast-grep --pattern 'enum LocalPermission {
  $$$
  whitelisted($_) {
    $$$
  }
  $$$
}'

#!/bin/bash
# Search for test files that might use this permission
fd "test.*\.rs$" --exec grep -l "LocalPermission::whitelisted"

#!/bin/bash
# Let's check the definition of LocalPermission to understand its structure
rg "enum LocalPermission" -A 10

#!/bin/bash
# Let's also check if there are any other variants of whitelisted usage
rg "whitelisted\(" -A 2

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Find test files that might need updates for start time functionality

# Look for test files containing permission-related tests
echo "Test files with permission-related tests:"
rg -l "LocalPermission::(blacklisted|whitelisted)" --type rust --glob "**/tests/**"

# Look for permission-related test functions
echo -e "\nPermission-related test functions:"
rg "fn.*test.*permission" --type rust --glob "**/tests/**"

#!/bin/bash
# Let's examine the permission-related code in the identified test files

echo "=== Checking marketplace_app.rs permissions ==="
rg "LocalPermission::(blacklisted|whitelisted)" -B 2 -A 2 tests-integration/tests/marketplace_app.rs

echo -e "\n=== Checking validator_staking.rs permissions ==="
rg "LocalPermission::(blacklisted|whitelisted)" -B 2 -A 2 tests-integration/tests/validator_staking.rs

# Let's also check for any permission-related assertions
echo -e "\n=== Checking permission-related assertions ==="
rg "assert.*Permission" tests-integration/tests/marketplace_app.rs tests-integration/tests/validator_staking.rs

Configuration used: CodeRabbit UI
Review profile: CHILL

contracts/ecosystem/andromeda-vault/src/contract.rs (7)

Line range hint 93-100: Enhance error handling in the reply function

Using unwrap_err() directly assumes that msg.result is an error, but it's safer to pattern match to handle both success and error cases explicitly. This prevents potential panics if the function evolves in the future.

Apply this diff to improve error handling:

-pub fn reply(_deps: DepsMut, _env: Env, msg: Reply) -> Result<Response, ContractError> {
-    if msg.result.is_err() {
-        return Err(ContractError::Std(StdError::generic_err(
-            msg.result.unwrap_err(),
-        )));
-    }
-
-    Ok(Response::default())
-}
+pub fn reply(_deps: DepsMut, _env: Env, msg: Reply) -> Result<Response, ContractError> {
+    match msg.result {
+        ContractResult::Ok(_) => Ok(Response::default()),
+        ContractResult::Err(err) => Err(ContractError::Std(StdError::generic_err(err))),
+    }
+}

---

Line range hint 242-245: Validate percent in percentage withdrawals to prevent logic errors

When handling WithdrawalType::Percentage(percent), ensure that percent is greater than zero and less than or equal to one. Failing to validate this can lead to unexpected withdrawal amounts or arithmetic errors.

Apply this diff to add validation for percent:

match withdrawal_type {
    WithdrawalType::Amount(amount) => { /* existing code */ }
    WithdrawalType::Percentage(percent) => {
+       ensure!(
+           percent > Decimal::zero() && percent <= Decimal::one(),
+           ContractError::InvalidWithdrawal {
+               msg: Some("Percent must be between 0 and 1 (exclusive)".to_string()),
+           }
+       );
        let amount = balance * percent;
        // rest of the code
    }
}

---

Line range hint 299-300: Review the unconditional error in query_balance when strategy is provided

The ensure!(false, ContractError::TemporarilyDisabled {}); line causes the function to always return an error when a strategy is specified. If this functionality is intended to be disabled temporarily, consider providing a more descriptive error message or implementing feature flags.

Apply this diff to improve clarity:

-ensure!(false, ContractError::TemporarilyDisabled {});
+return Err(ContractError::FeatureDisabled {
+    msg: Some("Querying balance with strategy is temporarily disabled.".to_string()),
+});

Alternatively, implement the required logic if ready.

---

Line range hint 316-328: Consider removing or documenting commented-out code

There is a block of commented-out code intended for future use. While it's acceptable during development, it can clutter the codebase over time. Consider removing it or adding a TODO comment with references to work items or issues.

---

Line range hint 353-372: Re-examine the ownership check in execute_update_strategy

The current implementation checks if the strategy's owner is the vault contract. Depending on the design, it might be more appropriate to check for operator permissions or implement a more flexible access control mechanism. This ensures that strategies managed by the vault are correctly authorized without requiring ownership transfer.

---

Line range hint 58-61: Improve error messages in handle_execute for unimplemented variants

Returning a generic NotImplemented error for ExecuteMsg::Withdraw may confuse users. Provide a clearer message or guide them to the correct function.

Apply this diff to enhance the error message:

-ExecuteMsg::Withdraw { .. } => Err(ContractError::NotImplemented {
-    msg: Some("Please use WithdrawVault".to_string()),
-}),
+ExecuteMsg::Withdraw { .. } => Err(ContractError::InvalidRequest {
+    msg: Some("Withdraw is deprecated; please use WithdrawVault instead.".to_string()),
+}),

---

Line range hint 78-82: Use _deps and _env consistently to indicate unused parameters

In the reply function, parameters are prefixed with an underscore to indicate they are unused. Ensure consistent naming to maintain code readability.

contracts/data-storage/andromeda-point/src/contract.rs (1)

72-73: LGTM! Minor formatting suggestion.

The migration function correctly implements the new signature with the env parameter and properly forwards it to the base contract's migrate function.

Consider adding a space after the comma in the migrate call for better readability:

-    ADOContract::default().migrate(deps,env, CONTRACT_NAME, CONTRACT_VERSION)
+    ADOContract::default().migrate(deps, env, CONTRACT_NAME, CONTRACT_VERSION)

contracts/modules/andromeda-date-time/src/contract.rs (1)

125-125: Fix spacing in migrate function call

Add a space after the comma for consistent formatting:

-    ADOContract::default().migrate(deps,env, CONTRACT_NAME, CONTRACT_VERSION)
+    ADOContract::default().migrate(deps, env, CONTRACT_NAME, CONTRACT_VERSION)

contracts/app/andromeda-app-contract/src/contract.rs (1)

200-201: LGTM! Minor formatting fix needed.

The signature change to use the env parameter is aligned with the PR objectives for time-based permission handling. However, there's a minor formatting issue.

Add a space after the comma in the migrate call:

-    ADOContract::default().migrate(deps,env, CONTRACT_NAME, CONTRACT_VERSION)
+    ADOContract::default().migrate(deps, env, CONTRACT_NAME, CONTRACT_VERSION)

contracts/finance/andromeda-splitter/src/contract.rs (1)

Line range hint 167-171: Document the remainder funds behavior

The code contains a comment questioning the behavior of remainder funds. This should be properly documented to clarify the intended behavior.

Add a documentation comment explaining that remaining funds are returned to the transaction sender:

+    // Any remaining funds (dust from rounding) are returned to the transaction sender
     if !remainder_funds.is_empty() {
         msgs.push(SubMsg::new(CosmosMsg::Bank(BankMsg::Send {
             to_address: info.sender.to_string(),
             amount: remainder_funds,
         })));

contracts/finance/andromeda-validator-staking/src/contract.rs (1)

105-106: Consider adding documentation about time-based permissions.

Since this change is part of implementing start time for permissions, it would be helpful to add documentation explaining how the migrate function interacts with the new time-based permission system.

+/// Migrates the contract to a new version.
+/// The env parameter is used by the base contract to validate time-based permissions.
 pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg) -> Result<Response, ContractError> {
     ADOContract::default().migrate(deps,env, CONTRACT_NAME, CONTRACT_VERSION)
 }

contracts/fungible-tokens/andromeda-lockdrop/src/contract.rs (1)

Line range hint 1-600: Consider leveraging existing time-based permission patterns.

This contract demonstrates several robust patterns for handling time-based permissions that could be valuable for implementing the start time feature mentioned in the PR objectives:

• Using MillisecondsExpiration for precise time handling
• Clear window-based permission logic with is_deposit_open, is_withdraw_open, and is_phase_over
• Proper time comparisons using is_in_past and plus_milliseconds

contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs (2)

Line range hint 744-754: Consider using checked comparison for payment validation

The payment validation in both CW20 and native token paths uses direct equality comparison (eq) after performing checked arithmetic. Consider using checked comparison throughout to maintain consistent overflow protection.

For the CW20 path:

-                ensure!(
-                    amount_sent.eq(&total_required_payment),
-                    ContractError::InvalidFunds {
+                ensure!(
+                    amount_sent == total_required_payment,
+                    ContractError::InvalidFunds {

For the native token path:

-                ensure!(
-                    amount_sent.eq(&total_required_payment.u128()),
-                    ContractError::InvalidFunds {
+                ensure!(
+                    Uint128::from(amount_sent) == total_required_payment,
+                    ContractError::InvalidFunds {

Also applies to: 828-838

---

Line range hint 379-380: Add validation for start time vs end time relationship

Currently, there's no validation to prevent setting a start time that occurs after the end time. This could lead to sales that never become active.

Consider adding validation in the execute_start_sale function:

     let end_expiration = if let Some(duration) = duration {
         ensure!(!duration.is_zero(), ContractError::InvalidExpiration {});
+        let end_time = start_time
+            .unwrap_or(Expiry::FromNow(Milliseconds::from_seconds(1)))
+            .get_time(&env.block)
+            .plus_milliseconds(duration);
+        ensure!(
+            start_expiration.get_time(&env.block) <= end_time,
+            ContractError::InvalidExpiration {}
+        );
         expiration_from_milliseconds(
             start_time
                 .unwrap_or(Expiry::FromNow(Milliseconds::from_seconds(1)))

packages/std/src/ado_contract/permissioning.rs (1)

865-938: Enhance test coverage for combined start and end time scenarios.

While the test comprehensively covers start time behavior, it should also verify:

1. Cases where both start and end times are set
2. Error cases for invalid time relationships (start time after end time)

Consider adding test cases like:

#[test]
fn test_permission_start_and_end_time() {
    let mut deps = mock_dependencies();
    let mut env = mock_env();
    let action = "action";
    let actor = "actor";
    let contract = ADOContract::default();
    
    // Test valid case: start_time < end_time
    let start = Expiry::AtTime(MillisecondsExpiration::from_seconds(1));
    let end = Expiry::AtTime(MillisecondsExpiration::from_seconds(2));
    
    let permission = Permission::Local(LocalPermission::Whitelisted {
        start: Some(start),
        expiration: Some(end),
    });
    
    // Test permission behavior at different times
    env.block.time = MillisecondsExpiration::from_seconds(0).into(); // Before start
    let res = contract.is_permissioned(deps.as_mut(), env.clone(), action, actor);
    assert!(res.is_err());
    
    env.block.time = MillisecondsExpiration::from_seconds(1).into(); // At start
    let res = contract.is_permissioned(deps.as_mut(), env.clone(), action, actor);
    assert!(res.is_ok());
    
    env.block.time = MillisecondsExpiration::from_seconds(2).into(); // At end
    let res = contract.is_permissioned(deps.as_mut(), env.clone(), action, actor);
    assert!(res.is_err());
}

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Check if other contracts have similar migrate function signatures
# Expected: All contract migrate functions should use the env parameter consistently

# Search for migrate function signatures in contract files
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, $msg: MigrateMsg)'

# Also check for any inconsistent signatures that might still have _env
ast-grep --pattern 'pub fn migrate(deps: DepsMut, _env: Env, $msg: MigrateMsg)'

#!/bin/bash
# Let's try a different approach using ripgrep to find migrate functions
# Search for migrate function implementations in contract.rs files
rg -U "pub fn migrate.*\{[\s\S]*?\}" --type rust -g "contract.rs" -A 5

# Also search specifically for ADOContract::default().migrate calls
rg "ADOContract::default\(\)\.migrate" --type rust

#!/bin/bash
# Description: Check migrate function signatures in other contracts
# Expect: All contracts should use the env parameter consistently

# Search for migrate function definitions
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, $msg: $_) -> Result<$_, $_>'

# Also check for any inconsistent usage with _env
ast-grep --pattern 'pub fn migrate(deps: DepsMut, _env: Env, $msg: $_) -> Result<$_, $_>'

#!/bin/bash
# Let's try a different approach using ripgrep to find migrate functions
# Search for migrate function implementations across the codebase
rg -U "pub fn migrate.*\{[\s\S]+?\}" --type rust -A 5

# Also search for ADOContract::default().migrate usage
rg "ADOContract::default\(\).migrate" --type rust

#!/bin/bash
# Description: Check if all migrate functions in the codebase follow the same pattern
# Expected: All migrate functions should use the env parameter consistently

ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, $msg: $_) -> Result<$_, $_>'

#!/bin/bash
# Let's try a different approach to find migrate functions
# First, let's find all Rust files that contain "migrate"
rg -l "migrate" --type rust

# Then, let's examine the migrate function implementations
rg "pub fn migrate.*\{" --type rust -A 2

#!/bin/bash
# Description: Check migration function signatures across contracts
# Expected: All migrate functions should accept and forward the env parameter

# Search for migrate function definitions
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, $msg: $_) -> Result<Response, ContractError> {
  $$$
}'

# Search for any potentially inconsistent migrate functions
ast-grep --pattern 'pub fn migrate(deps: DepsMut, $msg: $_) -> Result<Response, ContractError> {
  $$$
}'

#!/bin/bash
# Let's try a different approach using ripgrep to find migrate functions
# Search for migrate function definitions with and without env parameter

# Search for migrate functions with env parameter
rg "pub fn migrate.*deps.*env.*msg.*Result" -A 2

# Search for migrate functions without env parameter
rg "pub fn migrate.*deps.*msg.*Result" -A 2

#!/bin/bash
# Description: Check if all contracts have been updated to use the env parameter in migrate
# Expected: All migrate functions should use the env parameter consistently

# Search for migrate function definitions
ast-grep --pattern 'pub fn migrate(deps: DepsMut, _env: Env, _msg: MigrateMsg)'

# Search for migrate function calls to ensure they pass the env parameter
ast-grep --pattern 'ADOContract::default().migrate($deps, $env, $name, $version)'

#!/bin/bash
# Let's try a more flexible pattern to find migrate functions and their implementations

# Find migrate function definitions with any env parameter name
ast-grep --pattern 'pub fn migrate(deps: DepsMut, $env: Env, $msg: MigrateMsg)'

# Find migrate function definitions with underscore env
ast-grep --pattern 'pub fn migrate(deps: DepsMut, _env: Env, $msg: MigrateMsg)'

# Find migrate function calls with ADOContract
rg "ADOContract::default\(\).migrate" -A 1 -B 1

# Find all Rust files that might contain migrate functions
fd "contract.rs$" --exec grep -l "fn migrate"

#!/bin/bash
# Description: Check for consistency of migrate function signatures across contracts
# Expected: All migrate functions should use the env parameter

# Search for migrate function definitions
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg)'

# Search for migrate function calls to ensure they pass the env parameter
ast-grep --pattern 'ADOContract::default().migrate(deps, env, $_)'

#!/bin/bash
# Let's try a more flexible pattern matching approach

# Search for migrate function definitions with env parameter
rg "pub fn migrate.*deps.*env.*msg.*ContractError" -A 2

# Search for migrate function implementations using ADOContract
rg "ADOContract::default\(\)\.migrate" -A 1

#!/bin/bash
# Description: Check if other contracts have similar migrate function signatures
# Expect: All migrate functions should accept and use the env parameter

# Search for migrate function definitions in contract.rs files
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, $msg: $_) -> Result<$_, $_>'

#!/bin/bash
# Let's try a different approach to find migrate functions
# First, let's find all contract.rs files and then use ripgrep to search for migrate functions

# Find migrate function implementations with context
rg -U "pub fn migrate.*\{[\s\S]*?\}" -g "contract.rs" -A 5

#!/bin/bash
# Description: Verify migration function signatures across contracts
# Expect: All migrate functions should use the env parameter consistently

# Search for migrate function definitions
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, $msg: $_) -> Result<$_, $_>'

#!/bin/bash
# Let's try a different approach to find migrate functions
# First, let's find all Rust files in the contracts directory
fd "contract.rs" contracts/ --exec rg -l "pub fn migrate"

# Then, let's examine the migrate function implementations
fd "contract.rs" contracts/ --exec rg -A 3 "pub fn migrate.*DepsMut.*Env.*Result"

#!/bin/bash
# Search for LocalPermission struct definition and is_permissioned function
# to find where the start time feature is implemented

echo "Searching for LocalPermission struct definition..."
ast-grep --pattern 'struct LocalPermission {
  $$$
}'

echo "Searching for is_permissioned function implementation..."
ast-grep --pattern 'fn is_permissioned($$$) {
  $$$
}'

# Search for any test files related to permission start time
echo "Searching for permission start time tests..."
rg -l "test_permission_start_time"

#!/bin/bash
# Check the permissioning file content
echo "Checking permissioning.rs content..."
cat packages/std/src/ado_contract/permissioning.rs

# Search for any start_time related code in the codebase
echo "Searching for start_time related code..."
rg "start_time" -A 3

#!/bin/bash
# Search for LocalPermission struct and its implementation
echo "Checking LocalPermission struct definition..."
rg -A 10 "pub enum LocalPermission"

echo "Checking LocalPermission implementation..."
rg -A 10 "impl LocalPermission"

#!/bin/bash
# Check the full LocalPermission implementation including is_permissioned function
echo "Checking full LocalPermission implementation..."
rg -B 5 -A 20 "fn is_permissioned" packages/std/src/ado_base/permissioning.rs

#!/bin/bash
# Get the complete is_permissioned function implementation
echo "Getting complete is_permissioned implementation..."
rg -B 5 -A 50 "pub fn is_permissioned" packages/std/src/ado_base/permissioning.rs

#!/bin/bash
# Description: Verify that all contracts have consistent migrate function signatures
# Expected: All migrate functions should use the env parameter similarly

# Search for migrate function signatures across all contract files
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg)'

# Also check for any potentially inconsistent signatures
ast-grep --pattern 'pub fn migrate(deps: DepsMut, _env: Env, _msg: MigrateMsg)'

#!/bin/bash
# Let's try a different approach using ripgrep to search for migrate functions
# This will give us more context and handle potential variations in formatting

# Search for migrate function definitions with context
rg -U "pub fn migrate.*\{[\s\S]*?\}" -g "*.rs" -A 5

#!/bin/bash
# Description: Check for consistent migration function signatures across contracts
# Expect: All migrate functions should accept and use the env parameter

# Search for migrate function signatures
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg)'

# Search for migrate function calls to ensure env is passed
ast-grep --pattern 'ADOContract::default().migrate($_, env, $_, $_)'

#!/bin/bash
# Description: Check if other contracts have similar migration function changes
# Expected: Similar changes in other contract files

# Search for migrate function implementations
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg)'

# Search for ADOContract migrate calls
rg 'ADOContract::default\(\)\.migrate\(deps,\s*env,'

#!/bin/bash
# Description: Check the maximum number of recipients allowed and analyze the potential computational complexity

# Search for recipient limit constraints
echo "Searching for recipient limits..."
rg -A 2 "recipients\.len\(\)"

# Search for any existing DoS-related issues or discussions
echo "Checking for related DoS discussions..."
gh issue list --search "DoS splitter"

#!/bin/bash
# Description: Check for consistent migrate function signatures across contracts
# Expected: All migrate functions should use the env parameter similarly

# Search for migrate function definitions
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg)'

# Look for any potentially inconsistent migrate signatures
ast-grep --pattern 'pub fn migrate(deps: DepsMut, _env: Env, _msg: MigrateMsg)'

#!/bin/bash
# Let's try a more flexible pattern search using ripgrep to find migrate function implementations

# Search for migrate function definitions with various parameter patterns
rg -U "pub fn migrate.*\{[\s\S]*ADOContract::default\(\)\.migrate" --type rust

# Also search for any migrate function definitions to ensure we catch all variants
rg "pub fn migrate.*deps: DepsMut" --type rust

#!/bin/bash
# Description: Check for consistency of migrate function signature across contracts
# Expected: All migrate functions should use the env parameter similarly

# Search for migrate function definitions
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg)'

# Search for migrate function calls to ensure they pass the env parameter
ast-grep --pattern 'ADOContract::default().migrate(deps, env, $_, $_)'

 pub fn has_permission(storage: &dyn Storage, addr: &Addr) -> Result<bool, ContractError> {
     let is_operator = ADOContract::default().is_owner_or_operator(storage, addr.as_str())?;
+    // If operator, no need to check start time
+    if is_operator {
+        return Ok(true);
+    }
     let allowed = match RESTRICTION.load(storage)? {
         CounterRestriction::Private => is_operator,
         CounterRestriction::Public => true,
     };
-    Ok(is_operator || allowed)
+    Ok(allowed)
 }

        let owner = self.owner.load(deps.storage)?;

        // Clear all permissions
-       self.execute_clear_all_permissions(ExecuteContext::new(
+       self.execute_clear_all_permissions(ExecuteContext::new(
            deps.branch(),
            MessageInfo {
                sender: owner,
                funds: vec![],
            },
            env,
-       ))?;
+       )).map_err(|e| ContractError::Std(StdError::generic_err(format!("Failed to clear permissions: {}", e))))?;

#!/bin/bash
# Description: Check for existing migrations that might be affected by the permission clearing change

# Look for migrate function calls
rg -A 5 "fn\s+migrate" 

# Look for permission-related migrations
rg -A 5 "permission" --type rust | grep -i "migrate"

                AndromedaMsg::Permissioning(msg) => self.execute_permissioning(ctx, msg),
-               AndromedaMsg::AMPReceive(_) => panic!("AMP Receive should be handled separately"),
+               AndromedaMsg::AMPReceive(_) => Err(ContractError::InvalidOperation { 
+                   msg: Some("AMP Receive must be handled separately using execute_amp_receive".to_string())
+               }),

#!/bin/bash
# Description: Check for inconsistent migrate function signatures across contracts
# Expect: All migrate functions should have the same signature pattern

# Search for migrate function signatures
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg)'

#!/bin/bash
# Let's try a more flexible pattern to catch migrate function signatures
# This will help us identify any variations in the signature

# Search for migrate function signatures with ripgrep for better context
rg "pub fn migrate.*DepsMut.*Env.*MigrateMsg" -A 2

#!/bin/bash
# Description: Verify that other contracts have similar migrate function signatures
# Expected: All migrate functions should use the env parameter consistently

# Search for migrate function signatures across all contract files
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg)'

# Also check for any inconsistent signatures that might have been missed
ast-grep --pattern 'pub fn migrate(deps: DepsMut, _env: Env, _msg: MigrateMsg)'

#!/bin/bash
# Let's try a more flexible pattern to find migrate functions
# Search for migrate function implementations across contract files
rg -U "pub fn migrate.*\{[\s\S]*?\}" --type rust -g 'contract.rs'

# Also search for ADOContract::default().migrate usage
rg "ADOContract::default\(\)\.migrate" --type rust

    let end_expiration = if let Some(duration) = duration {
        ensure!(!duration.is_zero(), ContractError::InvalidExpiration {});
-       expiration_from_milliseconds(
+       let end_time = expiration_from_milliseconds(
            start_time
                // If start time isn't provided, it is set one second in advance from the current time
                .unwrap_or(Expiry::FromNow(Milliseconds::from_seconds(1)))
                .get_time(&env.block)
                .plus_milliseconds(duration),
-       )?
+       )?;
+       // Validate that start_time is before end_time
+       if let Some(start) = start_time {
+           ensure!(
+               start.get_time(&env.block) < end_time.get_time(&env.block),
+               ContractError::InvalidExpiration {}
+           );
+       }
+       end_time
    } else {
        Expiration::Never {}
    };

#!/bin/bash
# Description: Verify andromeda-std version update from 1.3.4 to 1.4.0

# Check Cargo.toml for andromeda-std dependency version
rg -U 'andromeda-std.*version.*=.*"1\.4\.0"' Cargo.toml

# Check for any remaining old versions
rg -U 'andromeda-std.*version.*=.*"1\.3\.4"' Cargo.toml

#!/bin/bash
# Let's check the actual content of Cargo.toml to see the andromeda-std dependency
fd Cargo.toml --exec cat {}

#!/bin/bash
# Let's check the specific Cargo.toml for andromeda-cw721 contract
cat contracts/non-fungible-tokens/andromeda-cw721/Cargo.toml

#!/bin/bash
# Check workspace-level andromeda-std version in root Cargo.toml
rg 'andromeda-std.*=.*path.*=.*"./packages/std"' Cargo.toml

#!/bin/bash
# Let's check the version in the andromeda-std package's Cargo.toml directly
cat packages/std/Cargo.toml | head -n 5

pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg) -> Result<Response, ContractError> {
    ADOContract::default().migrate(deps, env, CONTRACT_NAME, CONTRACT_VERSION)
}

#!/bin/bash
# Description: Check for consistency in migrate function signatures across contracts
# Expect: All migrate functions should accept and forward the env parameter

# Search for migrate function definitions
ast-grep --pattern 'pub fn migrate(deps: DepsMut, env: Env, $msg: $_) -> Result<$_, $_>'

# Search for migrate function calls to ensure env is forwarded
ast-grep --pattern 'migrate(deps, env, $_)'

#!/bin/bash
# Let's try a different approach to find migrate functions and their implementations

# Find all Rust files that might contain migrate functions
echo "=== Files containing 'migrate' function ==="
rg -l "fn\s+migrate" --type rust

echo -e "\n=== Migrate function implementations ==="
# Show the actual migrate function implementations with context
rg "fn\s+migrate.*deps.*->.*Result" -A 3 --type rust

echo -e "\n=== Migrate function calls ==="
# Look for migrate function calls
rg "\.migrate\(.*deps.*\)" --type rust

contracts/ecosystem/andromeda-vault/src/contract.rs (2)

393-394: LGTM! Consider adding migration documentation.

The migration function has been correctly updated to include and pass the environment context. This change aligns with the contract's evolution to support time-based features.

Consider adding a doc comment explaining the migration process and any state changes that occur during migration.

+/// Migrates the contract to a new version.
+/// 
+/// # Arguments
+/// 
+/// * `deps` - Mutable dependencies
+/// * `env` - Environment information
+/// * `_msg` - Migration message (unused)
 pub fn migrate(deps: DepsMut, env: Env, _msg: MigrateMsg) -> Result<Response, ContractError> {

---

Line range hint 1-500: Consider architectural implications of time-based permissions.

While the migration changes are in place, there are several areas that might need future updates to fully support time-based permissions:

1. Strategy management operations might need to respect permission start times
2. Withdrawal operations might need to verify permission timing
3. Permission checks in execute_update_strategy might need to consider start times

Consider creating follow-up tickets to track these potential updates. Would you like me to help create these issues?

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Find all permission and authorization checks in the codebase
# to ensure compatibility with the new time-based permission system.

# Search for permission checks
echo "Searching for permission-related function calls..."
rg -A 3 "is_contract_owner|is_operator|Unauthorized|NotAssignedOperator"

# Search for ADOContract usage that might involve permissions
echo "Searching for ADOContract usage..."
ast-grep --pattern 'ADOContract::default().$_($$$)'

#!/bin/bash
# Let's search for permission-related code in the vault contract and its dependencies

# Search for permission-related code in the vault contract
echo "=== Searching permission checks in vault contract ==="
rg -A 5 "is_contract_owner|is_operator|Unauthorized|NotAssignedOperator" contracts/ecosystem/andromeda-vault/

# Search for permission-related imports and usage
echo "=== Searching permission-related imports in vault contract ==="
rg "use.*ownership|use.*permission" contracts/ecosystem/andromeda-vault/

# Search for ADOContract usage in vault
echo "=== Searching ADOContract usage in vault ==="
ast-grep --pattern 'ADOContract::default().$_($$$)' contracts/ecosystem/andromeda-vault/

# Search for time-based permission code
echo "=== Searching time-based permission code ==="
rg -A 5 "expiration|start_time|end_time" contracts/ecosystem/andromeda-vault/

CHANGELOG.md (1)

90-90: Enhance the changelog entry with behavioral details.

While the entry correctly documents the addition of the start time feature, it would be more helpful to users if it included the key behavioral changes. Consider expanding it to:

-Permissions now have an optional start time [(#668)](https://github.com/andromedaprotocol/andromeda-core/pull/668)
+Permissions now have an optional start time with the following behavior [(#668)](https://github.com/andromedaprotocol/andromeda-core/pull/668):
+- Blacklisted permissions return true if start time hasn't been reached
+- Whitelisted permissions return the opposite of strict setting if start time hasn't been reached
+- End time checks are bypassed if start time hasn't been reached

contracts/math/andromeda-date-time/src/contract.rs (1)

124-125: LGTM: Standardized migrate function signature with potential for feature integration

The change correctly adds and forwards the env parameter to the ADOContract's migrate method. Given this contract's datetime functionality, consider if it could be utilized for the new permission start time feature.

Consider exposing utility functions from this contract that could be used by the permissions system for start time validation and comparison operations.

contracts/accounts/andromeda-fixed-multisig/src/contract.rs (1)

137-138: LGTM: Standardized migrate function signature with permission system considerations

The change correctly adds and forwards the env parameter to the ADOContract's migrate method. Given this contract's voter permission system, consider aligning its time-based functionality (max_voting_period) with the new permission start time feature for consistency in temporal permission handling across the platform.

Consider standardizing how temporal constraints are handled between the multisig voting system and the new permission start time feature to ensure a consistent user experience across different types of time-bound permissions.

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Check for consistent permission initialization across the codebase
rg -A 2 "LocalPermission::whitelisted\(None, None\)"

#[test]
fn test_permission_with_start_time() {
    // ... test setup ...
    
    // Test future start time
    let future_time = env.block.time.plus_seconds(100);
    address_list
        .execute_actor_permission(
            &mut router,
            owner.clone(),
            vec![AndrAddr::from_string(buyer.clone())],
            LocalPermission::whitelisted(Some(future_time), None),
        )
        .unwrap();
        
    // Verify permission is not active before start time
    let err = marketplace.execute_buy(...).unwrap_err();
    assert_eq!(err, ContractError::Unauthorized {});
    
    // Move time past start time
    router.set_block(BlockInfo {
        time: future_time.plus_seconds(1),
        ..router.block_info()
    });
    
    // Verify permission is active after start time
    marketplace.execute_buy(...).unwrap();
}

tests-integration/tests/permissions_orch.rs (2)

32-32: Remove unnecessary .clone() calls after .into_string()

When you call .into_string(), it consumes the Addr and returns a String. The subsequent .clone() is unnecessary and can be removed to simplify the code.

Here's how you can simplify the code:

32-         sender.clone().into_string().clone(),
+         sender.to_string(),

116-         owner: Some(sender.clone().into_string().clone()),
+         owner: Some(sender.to_string()),

172-                 actors: vec![AndrAddr::from_string(sender.clone().into_string())],
+                 actors: vec![AndrAddr::from_string(sender.to_string())],

Also applies to: 116-116, 172-172

---

185-204: Verify that permissions are correctly preserved after migration

Currently, the test only verifies that the number of permissions remains the same after migration by asserting assert_eq!(permissions.len(), 1). To ensure that permissions are fully preserved, consider asserting that the permissions before and after migration are identical.

Here's how you can enhance the test:

// Before migration
let permissions_before: Vec<PermissionInfo> = marketplace_juno
    .query(&QueryMsg::Permissions {
        actor: sender.clone().to_string(),
        limit: None,
        start_after: None,
    })
    .unwrap();

// Migrate
marketplace_juno.migrate(&MigrateMsg {}, new_code_id).unwrap();

// After migration
let permissions_after: Vec<PermissionInfo> = marketplace_juno
    .query(&QueryMsg::Permissions {
        actor: sender.to_string(),
        limit: None,
        start_after: None,
    })
    .unwrap();

// Assert that permissions are the same
assert_eq!(permissions_before, permissions_after);

This ensures that the permissions' details are preserved exactly through the migration process.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

CHANGELOG.md (1)

98-98: Document start time validation limitation

The changelog should mention that there's currently no validation to prevent users from setting a start time that occurs after the end time. This is an important limitation that users should be aware of.

-Permissions now have an optional start time [(#668)](https://github.com/andromedaprotocol/andromeda-core/pull/668)
+Permissions now have an optional start time [(#668)](https://github.com/andromedaprotocol/andromeda-core/pull/668). Note: Currently no validation between start and end times.

packages/std/src/ado_contract/permissioning.rs (3)

257-261: Consider adding event emission for permission clearing.

The clear_all_permissions function performs a destructive operation. While it's correctly restricted to owner access, adding event emission would help with tracking and auditing.

 pub fn clear_all_permissions(store: &mut dyn Storage) -> Result<(), ContractError> {
     permissions().clear(store);
+    // Consider emitting an event here
     Ok(())
 }

---

346-358: Add safety mechanism for clearing all permissions.

The execute_clear_all_permissions function could benefit from additional safety measures to prevent accidental execution.

Consider:

1. Adding a confirmation parameter
2. Implementing a two-step process
3. Adding detailed event attributes about the number of permissions cleared

---

865-938: Enhance test coverage for start time functionality.

While the test comprehensively covers basic start time scenarios, consider adding tests for:

1. Edge cases where start time equals end time
2. Cases where start time is after end time
3. Scenarios combining start and end times with different permission types

packages/std/src/ado_base/permissioning.rs (2)

Line range hint 101-144: Refactor duplicated start time logic in is_permissioned method

The start time checking logic is duplicated across all variants. Consider extracting this into a private helper method to improve maintainability and reduce code duplication.

Consider refactoring like this:

impl LocalPermission {
    fn check_start_time(&self, env: &Env) -> Option<bool> {
        match self {
            Self::Blacklisted { start, .. } |
            Self::Limited { start, .. } |
            Self::Whitelisted { start, .. } => {
                start.as_ref().map(|s| !s.get_time(&env.block).is_expired(&env.block))
            }
        }
    }
}

Then use it in is_permissioned:

if let Some(not_started) = self.check_start_time(env) {
    return if not_started {
        match self {
            Self::Blacklisted { .. } | Self::Limited { .. } => true,
            Self::Whitelisted { .. } => !strict,
        }
    }
}

---

228-235: Optimize cloning in get_permission method

The method unnecessarily clones the LocalPermission in both match arms.

Consider using references to avoid cloning:

 match self {
-    Self::Local(local_permission) => Ok(local_permission.clone()),
+    Self::Local(local_permission) => Ok(local_permission.to_owned()),
     Self::Contract(contract_address) => {
         let addr = contract_address.get_raw_address(&deps)?;
-        let local_permission = AOSQuerier::get_permission(&deps.querier, &addr, actor)?;
-        Ok(local_permission)
+        AOSQuerier::get_permission(&deps.querier, &addr, actor)
     }
 }

contracts/data-storage/andromeda-form/src/contract.rs (1)

167-168: Consider preserving permissions during migration

The migration function has been updated to use the env parameter, but there are concerns about permissions being cleared during migrations (see PR comments).

Consider implementing a permission preservation strategy during migrations. This could involve:

1. Backing up permissions before migration
2. Restoring permissions after migration
3. Adding validation to ensure permissions remain intact

Would you like help designing this preservation mechanism?

packages/std/src/ado_contract/execute.rs (1)

157-193: Address permission clearing during migration

Based on the PR comments from user crnbarr93, the current implementation still has the issue of clearing all permissioning whenever the user migrates. Consider implementing a more robust migration strategy that preserves permissions while updating their structure.

Consider these alternatives:

1. Implement a two-phase migration where permissions are first converted to the new structure before being cleared
2. Add a feature flag to control permission clearing during migration
3. Implement a rollback mechanism in case of migration failures

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

 pub fn get_permission(
-    &mut self,
+    &self,
     deps: Deps,
     actor: &str,
 ) -> Result<LocalPermission, ContractError> {

#!/bin/bash
# Description: Verify test coverage and migration handling

# Check for the mentioned test
echo "Checking for test_permission_start_time..."
rg -l "test_permission_start_time"

# Look for migration-related code
echo "Checking for migration-related code..."
rg -l "migrate.*Permission"

# Check for potential permission clearing in migrations
echo "Checking for permission clearing in migrations..."
rg -A 5 "fn migrate"

#!/bin/bash
# Check the test implementation in the permissioning file
echo "Checking test_permission_start_time implementation..."
rg -A 10 "test_permission_start_time" packages/std/src/ado_contract/permissioning.rs

# Look for other related test cases
echo "Checking for other permission start time related tests..."
rg "start.*time.*test|test.*start.*time" packages/std/src/ado_contract/permissioning.rs

#!/bin/bash
# Get the complete test implementation
echo "Getting complete test_permission_start_time implementation..."
rg -A 30 "fn test_permission_start_time" packages/std/src/ado_contract/permissioning.rs

# Check for any other permission-related test functions in the same file
echo "Checking other permission-related test functions..."
rg "^[[:space:]]*#\[test\].*\n.*fn.*permission" packages/std/src/ado_contract/permissioning.rs

#!/bin/bash
# Get the complete test_permission_start_time implementation
echo "Getting complete test_permission_start_time implementation..."
rg -A 50 "fn test_permission_start_time" packages/std/src/ado_contract/permissioning.rs

# Check for other permission-related test functions using multiline mode
echo "Checking other permission-related test functions..."
rg -U "#\[test\].*fn.*permission" packages/std/src/ado_contract/permissioning.rs