Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Improved macros and execution flow for AMP #741

Merged
merged 33 commits into from
Jan 22, 2025

Conversation

crnbarr93
Copy link
Contributor

@crnbarr93 crnbarr93 commented Dec 27, 2024

Motivation

These changes make pretty drastic updates to the Andromeda Standard crate's exposed macros. The reasoning for this is that maintenance of the growing amount of contracts was creating a lot of redundant overhead, it would be easy to miss adding specific checks (such as non-payable) to messages as well as an issue with access control via AMP packets.

The changes made here make handling AMP packets via a contract extremely easy and allows us to easily extend it's functionality in future without much overhead.

There are two major changes:

  1. We no longer directly handle AMPReceive messages, this is now done via a macro: andr_execute_fn. This macro unwraps and updates the ExecuteCtx object accordingly
  2. We can now tag ExecuteMsg variants with "attributes". These allow definition of checks at the message definition layer instead of in the contract execution flow, severely reducing redundant checks.

Other notable changes

  • To better fit the definition, our integration tests library has been renamed to e2e-tests.
  • The CW721 contract now has much cleaner execution flow, including a macro to validate access to the mint procedure
  • Tests were added for AMP access control to validate the macro changes

Implementation

To help understand the changes they'll be broken down in to sections.

Andromeda Execute Function

Previously our method of handling AMPReceive messages was to handle it directly via a standard message handler. This created an odd situation where the actual execution message handling was done in a separate function. This has been reworked to now be a standard macro around an execute function:

#[andr_execute_fn]
pub fn execute(ctx: ExecuteContext, msg: ExecuteMsg) -> Result<Response, ContractError> {
  // Standard execution flow...
}

It's worth noting that the parameters to this function are different from a standard execute call as they use the provided ExecuteContext struct. Any ADO specific messages still need to be handled but otherwise this is not far from a standard contract implementation.

This macro will both unwrap an AMP packet and also validate the provided message against any message attributes (see next section). As such the ExecuteContect struct has been expanded:

pub struct ExecuteContext<'a> {
    pub deps: DepsMut<'a>,
    pub info: MessageInfo,
    pub env: Env,
    pub amp_ctx: Option<AMPPkt>,
    pub contract: ADOContract<'a>,
    pub raw_info: MessageInfo,
}

It now includes a contract instance (to help reduce struct creation calls) and the original message info in the form of raw_info.

Message Attributes

There are a lot of cases where we want messages to have specific validation checks, these validation checks are usually extremely common and add a bit of overhead to implement and validate. To reduce this some of these redundant checks have been moved to "attributes". These are field attributes on the ExecuteMsg variants that define what validation should occur when the message is called (even when it is called via AMP). These include:

  1. nonpayable - This message should not accept funds
  2. restricted - This message should only be callable by the contract owner
  3. direct - This messae should not be callable via AMP

These can be added like so:

pub enum ExecuteMsg {
  #[attrs(restricted, nonpayable)]
  MyRestrictedMsg { //.. }.
  #[attrs(direct)]
  MyDirectMsg { //.. },
}

These can be applied in any particular order or combination. The checks for these are then performed using the andr_execute_fn macro so they do not require any extra code, however they do add methods to the ExecuteMsg enum that may be used as necessary.

Testing

Unit and integration tests were added as necessary.

Version Changes

andromeda-std: 1.5.0-b.2 -> 1.5.0-b.3
andromeda-macros: 1.0.0 -> 1.1.0-b.1
andromeda-splitter: 2.3.0-b.1 -> 2.3.0-b.2
andromeda-cw721: 2.5.0 -> 2.5.1-b.1

Notes

These changes are fairly substantial and may require paired review.

Future work

The message attributes should be extended to be attached to the generated contract schemas for easy frontend use. It may also be worth considering permissioning based attributes.

Summary by CodeRabbit

  • New Features

    • Renamed workspace member from "tests-integration" to "e2e-tests".
    • Added new dependencies for improved functionality and testing.
    • Introduced new test cases to validate permission-based access control and crowdfunding functionality.
  • Bug Fixes

    • Adjusted parameter order in multiple method calls to ensure proper functionality.
  • Documentation

    • Enhanced comments and documentation for clarity on new features and changes.
  • Refactor

    • Streamlined permission handling and context management across various contracts.
    • Modularized macro implementations for improved organization and maintainability.
  • Tests

    • Expanded testing coverage for various functionalities, ensuring robust validation of contract behaviors.
  • Chores

    • Updated version numbers and dependency configurations to maintain project standards.

Copy link
Contributor

coderabbitai bot commented Dec 27, 2024

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request introduces a comprehensive set of changes across the Andromeda protocol's codebase, focusing on macro improvements, permission handling, and code restructuring. The modifications span multiple packages and contracts, with key changes including updates to macro implementations, standardization of permission action method signatures, and introduction of new attribute handling mechanisms. The changes aim to enhance code modularity, improve error handling, and provide more flexible execution contexts across different contract components.

Changes

File/Module Change Summary
Cargo.toml Renamed workspace member from "tests-integration" to "e2e-tests"
Makefile Updated test target to use "e2e-test" instead of "integration-test"
Contracts (Multiple) Updated permission_action method calls to change parameter order
packages/std/macros Introduced new attribute handling modules, enhanced macro system
packages/std/src/common/context.rs Added new fields to ExecuteContext
Dependency Updates Updated andromeda-macros version to "1.0.1-b.1"

Suggested Reviewers

  • cowboy0015
  • joemonem

Poem

🐰 A Rabbit's Macro Melody 🐰

In Andromeda's digital grove,
Where macros dance and code they drove,
Parameters shifted with grace and might,
Permissions aligned, everything right!
A refactored world, both clean and bright!


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@crnbarr93
Copy link
Contributor Author

@CodeRabbit pause

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (22)
packages/std/macros/src/attrs/mod.rs (1)

16-49: Validate graceful handling for non-enum input.

Panic is acceptable for macro-derivation constraints. However, consider using compile_error!() instead of panic!() for clearer compile-time feedback.

-        _ => panic!("Attrs can only be derived for enums"),
+        _ => compile_error!("Attrs can only be derived for enums");
packages/andromeda-non-fungible-tokens/src/cw721.rs (1)

123-172: Graceful error handling with TryFrom conversion.

  • Returning Err("Unsupported message".to_string()) clarifies invalid conversions.
  • For more granular errors, consider specialized enums or typed error messages instead of plain strings. This fosters better debugging in large codebases.
-            _ => Err("Unsupported message".to_string()),
+            _ => Err(format!("Unsupported ExecuteMsg variant: {:?}", msg)),
contracts/non-fungible-tokens/andromeda-cw721/src/contract.rs (6)

1-1: Remove unnecessary wildcard usage in use statements
You can selectively import items instead of using an entire module or wildcard if certain items are never used. This helps reduce code clutter.


56-57: Check result of permission_action
Since permission_action may fail, consider wrapping it with error context or confirmations (e.g., logging) for easier debugging, although returning ContractError might be sufficient depending on your logging strategy.


95-95: Transparent naming
execute_transfer is straightforward. If you foresee multiple “transfer” flows (direct transfer, partial transfer, or some specialty logic), naming them more specifically can prevent confusion.


114-114: Arithmetic or subsequent calls
Consider whether any additional signals (events/attributes) should be added to res prior to returning. For example, you might include the final message handling route for better debugging.


219-223: Context destructuring
When destructuring ExecuteContext, consider whether you need a separate handle on ctx.env or ctx.info for logging or verification. Sometimes retaining the full ctx is more flexible.


241-241: Hardcoded "Transfer"
Reusing "Transfer" as a key might become confusing if new transfer-like actions are introduced. Consider extracting into a constant or enumerating these actions for clarity.

e2e-tests/tests/crowdfund_app.rs (1)

112-112: Kernel address parameter
Verifying the correctness or existence of the kernel address in your integration environment is important. You might want to confirm it with andr.kernel.addr() before usage.

packages/std/macros/src/attrs/utils.rs (1)

26-37: Recommend adding tests for edge cases in process_variants_dynamic.
Although the logic is straightforward, consider testing scenarios such as enums with no variants, multiple attributes, or invalid attributes to ensure reliability.

Would you like me to open a new issue to track test coverage for these scenarios?

packages/std/macros/src/attrs/direct.rs (2)

20-39: Prevent potential panics or partial parse failures in check_attribute.
While using .unwrap_or(()) is fine for minimal error handling, consider adding at least a debug-level log or returning a bound error to aid troubleshooting if the argument parse fails.

.attr.parse_args_with(|input: syn::parse::ParseStream| {
    ...
})
.unwrap_or(())

41-59: Encourage DRY in generate_impl across different attributes.
Multiple attributes (Direct, NonPayable, Restricted) create functions with similar patterns. Consider extracting the shared logic into a helper function or macro to maintain DRY principles and uniform code generation.

packages/std/macros/src/attrs/payable.rs (2)

20-39: Check for consistent usage of error handling.
Similar to the DirectAttribute’s approach, consider returning a dedicated error or logging an appropriate message for improved clarity if the attribute parsing fails unexpectedly.


41-59: Add clarifying comments on the difference between "payable" and "nonpayable".
It might be beneficial to enhance maintainability by explicitly describing that “nonpayable” means no funds should accompany the message, hence is_payable returns the negation of the attribute. This is straightforward but worth documenting for future developers.

packages/std/macros/src/attrs/restricted.rs (1)

42-60: Confirm that the generated is_restricted function aligns with future ACL expansions.
This method might need to work in tandem with additional ACL logic in the future. Consider adding forward-compatible notes or extension points to handle roles or group-based restrictions if that’s in the roadmap.

packages/std/macros/src/query.rs (1)

5-54: Consider adding doc comments or inline explanation
While the functionality is clear (merging enum variants optionally based on the rates feature), adding higher-level doc comments would help later readers understand how/why these variants are merged. Also consider describing the conditional inclusion of Rates and AllRates more explicitly in inline comments.

e2e-tests/tests/macro_tests/exec_fn.rs (3)

14-27: Enum variant attribute approach looks extensible.

Each variant includes attributes like restricted, nonpayable, and direct, which help enforce constraints at the message-level. This is a clean approach to reduce repetitive checks. Make sure to include negative tests that confirm misapplied attributes are handled gracefully.


34-57: Test environment setup is comprehensive.

Using ADOContract::default().instantiate to initialize your contract ensures a realistic environment. Consider expanding the fixture in the future to load additional test data or permissions if needed for more complex testing scenarios.


104-106: Function is a neat helper for generating error contexts.

direct_msg_error is concise, reducing code duplication throughout the file.

packages/std/Cargo.toml (1)

49-49: Consider documenting test migration strategy

The addition of rstest suggests a move towards more modern testing practices. Consider documenting the testing strategy for the new macro functionality.

Would you like me to help create a testing guide in the documentation?

e2e-tests/Cargo.toml (1)

Line range hint 1-190: Consider cleaning up commented-out dependencies.

The file contains numerous commented-out dependencies and test entries. Consider removing these to improve maintainability and reduce confusion.

packages/std/src/amp/messages.rs (1)

267-271: Add documentation for the with_origin method.

Consider adding documentation explaining the purpose and usage of this method, similar to other methods in the struct.

+    /// Updates the origin field in the packet's context with the provided value.
+    /// This is useful when you need to modify the origin after the packet has been created.
+    /// 
+    /// # Arguments
+    /// * `origin` - The new origin value to set
+    /// 
+    /// # Returns
+    /// A new AMPPkt with the updated origin
     pub fn with_origin(&self, origin: impl Into<String>) -> AMPPkt {
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3e5fefa and 80b337b.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (46)
  • Cargo.toml (3 hunks)
  • Makefile (1 hunks)
  • contracts/data-storage/andromeda-boolean/src/contract.rs (1 hunks)
  • contracts/data-storage/andromeda-form/src/contract.rs (1 hunks)
  • contracts/finance/andromeda-splitter/src/contract.rs (6 hunks)
  • contracts/finance/andromeda-validator-staking/src/contract.rs (1 hunks)
  • contracts/fungible-tokens/andromeda-merkle-airdrop/src/contract.rs (1 hunks)
  • contracts/math/andromeda-curve/src/contract.rs (1 hunks)
  • contracts/math/andromeda-matrix/src/contract.rs (1 hunks)
  • contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (2 hunks)
  • contracts/non-fungible-tokens/andromeda-crowdfund/src/contract.rs (2 hunks)
  • contracts/non-fungible-tokens/andromeda-crowdfund/src/testing/tests.rs (1 hunks)
  • contracts/non-fungible-tokens/andromeda-cw721/src/contract.rs (11 hunks)
  • contracts/os/andromeda-ibc-registry/src/contract.rs (1 hunks)
  • contracts/os/andromeda-kernel/src/contract.rs (1 hunks)
  • contracts/os/andromeda-kernel/src/ibc.rs (1 hunks)
  • e2e-tests/Cargo.toml (2 hunks)
  • e2e-tests/tests/amp/access_control.rs (1 hunks)
  • e2e-tests/tests/amp/mod.rs (1 hunks)
  • e2e-tests/tests/crowdfund_app.rs (3 hunks)
  • e2e-tests/tests/macro_tests/attrs.rs (1 hunks)
  • e2e-tests/tests/macro_tests/exec_fn.rs (1 hunks)
  • e2e-tests/tests/macro_tests/mod.rs (1 hunks)
  • e2e-tests/tests/mod.rs (1 hunks)
  • packages/andromeda-finance/src/splitter.rs (1 hunks)
  • packages/andromeda-non-fungible-tokens/src/cw721.rs (1 hunks)
  • packages/std/Cargo.toml (2 hunks)
  • packages/std/macros/Cargo.toml (2 hunks)
  • packages/std/macros/src/attrs/direct.rs (1 hunks)
  • packages/std/macros/src/attrs/handler.rs (1 hunks)
  • packages/std/macros/src/attrs/mod.rs (1 hunks)
  • packages/std/macros/src/attrs/payable.rs (1 hunks)
  • packages/std/macros/src/attrs/restricted.rs (1 hunks)
  • packages/std/macros/src/attrs/utils.rs (1 hunks)
  • packages/std/macros/src/execute.rs (1 hunks)
  • packages/std/macros/src/instantiate.rs (1 hunks)
  • packages/std/macros/src/lib.rs (3 hunks)
  • packages/std/macros/src/query.rs (1 hunks)
  • packages/std/macros/src/utils.rs (1 hunks)
  • packages/std/src/ado_contract/execute.rs (1 hunks)
  • packages/std/src/ado_contract/permissioning.rs (9 hunks)
  • packages/std/src/amp/messages.rs (1 hunks)
  • packages/std/src/common/context.rs (1 hunks)
  • packages/std/src/common/denom.rs (1 hunks)
  • packages/std/src/error.rs (1 hunks)
  • packages/std/src/lib.rs (2 hunks)
✅ Files skipped from review due to trivial changes (2)
  • e2e-tests/tests/amp/mod.rs
  • e2e-tests/tests/macro_tests/mod.rs
🔇 Additional comments (82)
packages/std/macros/src/attrs/mod.rs (2)

1-5: Good modular organization.

These module declarations logically separate concerns (direct, handler, payable, restricted, utils), improving readability and reducing complexity.


7-15: Ensure these imports are genuinely needed.

All imported modules and crates appear to be in use, but confirm there are no superfluous imports of macros or traits. Keeping imports lean helps maintain clarity.

✅ Verification successful

Let me generate more targeted scripts to verify the imports in the specific file.


All imports are actively used in the module

Based on the search results, every import in the file is being used:

  • DirectAttribute is used in the module for attribute handling (line 27)
  • AttributeHandler is used as a trait for attribute handlers
  • NonPayableAttribute is instantiated in a Box (line 25)
  • TokenStream is used for macro input/output processing (line 45)
  • quote macro is imported for code generation
  • RestrictedAttribute is used in attribute handling (line 26)
  • parse_macro_input and DeriveInput are used for parsing macro input (line 17)
  • process_variants_dynamic is called to process enum variants (line 34)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all imports in this file are used.
ast-grep --pattern $'use $_::*' 

Length of output: 3842


Script:

#!/bin/bash
# Search for usage of DirectAttribute
ast-grep --pattern 'DirectAttribute'

# Search for usage of AttributeHandler
ast-grep --pattern 'AttributeHandler'

# Search for usage of NonPayableAttribute
ast-grep --pattern 'NonPayableAttribute'

# Search for usage of TokenStream
ast-grep --pattern 'TokenStream'

# Search for usage of quote macro
ast-grep --pattern 'quote!'

# Search for usage of RestrictedAttribute
ast-grep --pattern 'RestrictedAttribute'

# Search for usage of parse_macro_input and DeriveInput
ast-grep --pattern 'parse_macro_input!'
ast-grep --pattern 'DeriveInput'

# Search for usage of process_variants_dynamic
ast-grep --pattern 'process_variants_dynamic'

Length of output: 2800

packages/std/macros/src/lib.rs (6)

1-5: Solid file structure with multiple macro modules.

Defining specialized modules (attrs, execute, instantiate, query, utils) centralizes macro logic. This separation of concerns promotes maintainability.


17-18: Inline forwarding to execute::enum_implementation.

The andr_exec function succinctly delegates logic to execute::enum_implementation. This is clear and keeps lib.rs minimal.


21-36: Robust documentation for andr_execute_fn.

The doc comments thoroughly explain how the AMP unwrapping replaces ctx.info.sender with origin. This reduces confusion and fosters correct usage.


47-48: Consistent approach for andr_instantiate.

Likewise, you maintain a consistent interface, delegating the implementation to instantiate::enum_implementation. This uniform structure is beneficial for maintainability.


56-57: Clear separation of concerns for andr_query.

Same pattern of delegation indicates a well-thought-out design. This persists the code’s hierarchy of responsibilities.


59-79: Comprehensive macro documentation for derive_execute_attrs.

The doc block clearly explains the supported attributes (nonpayable, restricted, direct) and usage examples. This fosters correct, discoverable usage.

packages/std/macros/src/execute.rs (3)

50-60: Use of #[derive(::andromeda_std::AsRefStr, ::andromeda_std::ExecuteAttrs)].

Automatically deriving these traits is concise and reduces boilerplate. Be sure all expected custom attributes are recognized by the new macro expansions.


7-48: Conditional variant merging might cause confusion if features are toggled mid-development.

Ensure thorough testing when toggling the "rates" feature to confirm merging logic remains stable. Also verify that Right enum variants are consistently handled downstream.

✅ Verification successful

Conditional feature handling is properly implemented and tested

The codebase shows consistent and well-structured handling of the rates feature:

  • The RatesMessage enum and its variants are properly defined in packages/std/src/ado_base/rates.rs
  • Multiple contracts (auction, marketplace, string-storage, primitive, boolean, etc.) correctly implement rate-related functionality with proper feature checks
  • E2E tests in e2e-tests/tests/rates_orch.rs and e2e-tests/tests/kernel_orch.rs verify the rates functionality
  • All implementations properly handle both SetRate and RemoveRate messages
  • The conditional merging in the macro is consistent with how the feature is used throughout the codebase

The current implementation ensures that the rates feature toggle is handled safely and consistently across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check references to `RatesMessage` in the codebase to ensure they handle toggling of the "rates" feature.
rg -A 5 'RatesMessage'

Length of output: 24620


62-106: Enforces restricted/payable checks with clarity.

  • The code uses msg.is_restricted() and msg.is_payable() to gate function access and enforce payment features. Good alignment with the ExecuteAttrs model.
  • Confirm thorough negative path tests around these checks, especially unauthorized calls and non-empty funds for nonpayable messages.
✅ Verification successful

The search results show extensive test coverage for both unauthorized access and non-payable message handling across multiple contracts. Particularly relevant are the test cases in e2e-tests/tests/macro_tests/exec_fn.rs which specifically test the restricted and non-payable attributes:

  1. Tests verify unauthorized access is blocked:
(ExecuteMsg::RestrictedNonPayable, ATTACKER, Some(ContractError::Unauthorized {}), Some(ContractError::Unauthorized {}))
  1. Tests verify non-payable messages reject funds:
(ExecuteMsg::RestrictedNonPayable, OWNER, None, Some(ContractError::Payment(PaymentError::NonPayable {})))
  1. Tests cover combinations of restricted/non-payable attributes:
  • RestrictedNonPayable (both restricted and nonpayable)
  • RestrictedDirect (restricted only)
  • NonPayableDirect (nonpayable only)

The restricted/payable checks are thoroughly tested with proper error handling.

  • The test suite validates both positive and negative cases
  • Error paths are explicitly tested for both unauthorized access and payment validation
  • Tests cover all combinations of the restricted and payable attributes
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for relevant tests in the codebase
rg -A 15 'ExecuteMsg::.*restricted|ExecuteMsg::.*payable' 

Length of output: 59


Script:

#!/bin/bash
# Let's try a different approach to find test files and implementations
# First, let's look for test modules and files
fd "test" --type f --extension rs

# Then let's search for any test cases involving unauthorized access or payment errors
rg -A 10 "ContractError::Unauthorized|PaymentError::NonPayable"

# Also search for test attributes and test modules
ast-grep --pattern 'mod tests {
  $$$
}'

Length of output: 71976

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

9-10: Macro usage is aligned with new patterns.

The #[andr_execute_fn] import readies the contract for handling AMP unwrap internally. Good step toward standardizing the codebase’s approach.


96-97: Direct usage of raw_info.sender.

This is beneficial for preserving the original sender in the context of AMP. It's consistent with the documented approach that AMP modifies ctx.info.sender.


Line range hint 76-89: Fallback to ADOContract::default().execute(ctx, msg) in _ => ....

This fallback is a coherent approach for unhandled variants, but ensure you have clear isolation of “internal” vs. “external” messages.

✅ Verification successful

Let me try a different search strategy to understand the ADOContract implementation and its execute function.


Let me search for the ADOContract implementation in the standard package since it seems to be defined there.


Based on the implementation, I can now provide a final response about the fallback handling.

The fallback to ADOContract::default().execute() is safe and well-designed

The fallback handling in the contract is secure because:

  1. The ADOContract::execute() implementation only handles a specific set of well-defined AndromedaMsg variants:

    • Ownership management
    • App contract updates
    • Kernel address updates
    • Permissioning
    • Rates (when feature enabled)
  2. Any message that doesn't match these predefined variants results in a NotImplemented error, providing a secure default rejection for unknown messages.

  3. The implementation includes proper ownership and permission checks before executing sensitive operations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Confirm that fallback handle in ADOContract doesn't degrade security or block usage
rg -A 10 'fn execute\(.*ADOContract'

Length of output: 38


Script:

#!/bin/bash
# Search for ADOContract implementation and execute function
ast-grep --pattern 'impl ADOContract {'

# Also search for any struct definition
ast-grep --pattern 'struct ADOContract'

# Search for any imports or uses of ADOContract
rg "use.*ADOContract"

Length of output: 5829


Script:

#!/bin/bash
# Look for ADOContract implementation in the standard package
cat packages/std/src/ado_contract/state.rs

# Also check the execute implementation
cat packages/std/src/ado_contract/execute.rs

Length of output: 17710


275-275: Ensure lock_time expiration logic is thoroughly tested.

This logic ensures the contract is not updated while locked. Verify that lock expiration corner cases (e.g., block time matches exactly) pass tests.

✅ Verification successful

Let me search for more specific information about the lock time tests.


Let me check the actual implementation of the lock time logic.


Let me check one more thing about the lock expiry check implementation.


Let me try one final search to locate the lock expiry logic.


Lock time expiration logic is well-tested with comprehensive coverage

Based on the test files, the lock time expiration logic is thoroughly tested with various scenarios:

  • Tests cover both Expiry::FromNow and Expiry::AtTime variants
  • Multiple time ranges are tested: 1 day (86400000ms), 20 days (1728000000ms), and 1 year (31708800000ms)
  • Edge cases are covered with specific timestamps (e.g., 1724934977000ms, 1788006977000ms)
  • Tests include scenarios for both expired and non-expired locks

The implementation appears to be robust and well-tested for lock expiration corner cases.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for test coverage on 'execute_update_recipients' and lock_time
rg -A 5 'execute_update_recipients'

Length of output: 9923


Script:

#!/bin/bash
# Search for lock time related tests and implementation
rg -A 10 'test_execute.*lock' contracts/finance/andromeda-splitter/src/testing/

Length of output: 1049


Script:

#!/bin/bash
# Search for lock time implementation in the contract
rg -A 15 'execute_update_lock|fn is_locked' contracts/finance/andromeda-splitter/src/contract.rs

Length of output: 1142


Script:

#!/bin/bash
# Search for lock expiry check implementation
rg -A 10 'is_locked|lock_expired' contracts/finance/andromeda-splitter/src/

Length of output: 77


Script:

#!/bin/bash
# Search for Expiry type usage and implementation
ast-grep --pattern 'struct Expiry { $$$ }'
rg -A 5 'Expiry::|!splitter.lock_time.is_expired' contracts/finance/andromeda-splitter/src/

Length of output: 4927

contracts/non-fungible-tokens/andromeda-cw721/src/contract.rs (10)

16-17: Validate usage of is_context_permissioned
Ensure that calls to is_context_permissioned are consistently handled, and consider adding tests to confirm that invalid permission settings produce the correct error paths.

Also applies to: 19-19


33-33: Constant name clarity
MINT_ACTION is clear, but any future project-specific action strings would be more self-documenting if grouped or referenced from a single enum or well-defined set of action constants.


75-76: Entry point macro adoption
Using #[andr_execute_fn] is a good approach for standardizing your execution context. Ensure that no test coverage is lost if any existing tests were mocking direct calls to execute.


90-91: Batch mint invocation
Ensure that execute_mint and execute_batch_mint have identical permission handling if desired. The separation of single vs. batch logic is fine, but keep them in sync regarding authentication.


107-112: Fallback logic
Falling back to ADOContract::default().execute is a nice approach for unrecognized messages. However, ensure that any newly introduced ExecuteMsg variants are carefully validated before letting them pass through.


125-145: Macro-based permission check
This macro usage is highly readable. However, consider if you should track usage counts or if minters can lose permission mid-lifecycle. If that’s a possibility, re-check the stored data each time.


146-146: No meaningful change
This blank line or minimal insertion does not alter functionality.


154-154: Essential permission check
ensure_can_mint!(ctx); is good. Make sure you always call it at the top of the function, since partial modifications prior to the call could put the state in a half-committed position if the check fails.


194-194: Batch flow
Similarly, ensure_can_mint!(ctx); in the batch path is consistent with the single-mint approach. Great job maintaining consistent permission checks.


202-203: Avoid mutating ctx.amp_ctx repeatedly
Reassigning ctx.amp_ctx within a loop might increase the risk of overwriting needed context if multiple messages are processed. Verify that each iteration uses the correct cloned context.

e2e-tests/tests/crowdfund_app.rs (3)

13-13: Extended TokenExtension
Adding ExecuteMsg enhances your crowdfund’s capabilities. Keep an eye on versioning or changes to CW721 compliance if the extension diverges from standard usage.


19-19: Permissioning usage
Adding permission messages to the crowdfund logic is beneficial. Consider concurrency aspects if multiple permission requests happen at once (e.g., a scenario with multiple owners or high-frequency calls).


241-257: Permission actions
Lines 241–257 demonstrate setting the “Mint” permission and whitelisting the crowdfund contract. This is a solid approach to fine-grained access. Ensure that revoking permissions is equally straightforward if needed.

packages/std/macros/src/attrs/handler.rs (1)

1-12: AttributeHandler trait introduction
Excellent design to unify attribute checks and code generation. Make sure each implementing struct logs or errors on unknown attributes to prevent silent misconfiguration.

e2e-tests/tests/mod.rs (1)

33-38: New test modules
Adding amp and macro_tests is a good step for coverage. Ensure these modules run by default in your CI pipeline to catch issues early.

packages/std/src/lib.rs (2)

8-8: Replacing andr_exec with andr_execute_fn
Switching to andr_execute_fn is consistent with the new context-based pattern. Confirm any older references to andr_exec have been fully removed to avoid confusion or partial usage.


17-19: Re-exporting ExecuteAttrs
Exposing ExecuteAttrs makes it simpler for downstream code to derive attributes from an enum. Document the usage thoroughly to help integrators adopt these macros quickly.

e2e-tests/tests/macro_tests/attrs.rs (3)

6-17: Well-structured enum with clear attribute usage.

Using #[andr_exec] and #[cw_serde] together is a robust approach, and the attribute macros (#[attrs(...)]) are clearly demonstrating how different constraints (restricted, nonpayable, direct) can be applied. This design nicely centralizes the logic for attribute checks in a single location, which should simplify maintenance as the codebase evolves.


19-23: Comprehensive test data coverage.

By enumerating all permutations of TestEnum in the test cases, you ensure each variant’s attributes (restricted, payable, direct) are correctly asserted, minimizing the likelihood of missed behaviors. This thoroughness is commendable.


24-33: Clear and succinct test logic.

Each assertion (is_restricted(), is_payable(), must_be_direct()) aligns intuitively with the attributes defined for the corresponding enum variant. The code is succinct and easy to read, reducing future debugging overhead.

packages/std/macros/src/utils.rs (1)

7-29: Robust variant-merging utility with appropriate error handling.

  1. Merging two enums:
    • The logic in merge_variants properly checks for enum data, yielding a compile error if not valid. This helps maintain type safety.
  2. Extending variants is straightforward and clearly structured, reducing complexity for future modifications.
  3. The function name is clear and purposeful, aligning with standard naming conventions.
packages/std/src/common/context.rs (2)

9-10: Enhances contextual data with contract and raw_info fields.

Adding pub contract: ADOContract<'a> and pub raw_info: MessageInfo extends the ExecuteContext to store important data about the contract and the original message info. This improves readability and maintainability since downstream functions can leverage these fields without additional parameters.


18-22: Ensures cloned sender info is retained separately.

Cloning info allows the original context to be preserved in raw_info while permitting any direct usage or modifications to info. This design helps avoid unintended side-effects and keeps the code’s intention clear.

packages/std/macros/src/instantiate.rs (1)

5-29: Efficient struct enhancement with new fields.

  1. Automatically injecting kernel_address and owner fields via the procedural macro solidifies consistent data across all structs that use it.
  2. The straightforward usage of quote! captures the end structure clearly, reducing confusion.
  3. The panic for non-struct data is an appropriate fail-fast approach, promoting correct macro usage.
packages/std/macros/src/attrs/restricted.rs (1)

21-39: Validate attribute arguments robustly.
If users accidentally provide #[attrs(restricted, something_else)], it might cause confusion. Currently, the code only checks for restricted. Consider either erroring out or ignoring unexpected keywords to avoid confusion.

packages/std/macros/src/query.rs (1)

1-4: Imports look consistent and minimal
The chosen imports (e.g., merge_variants, quote, proc_macro::TokenStream) are clear, relevant, and sufficiently minimal for this file. No issues found.

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

50-50: New parameter order is aligned with recent refactoring
The updated call to permission_action(deps.storage, SET_DELETE_VALUE_ACTION) matches the refactored function signature and is consistent with changes in other files. Looks correct.

e2e-tests/tests/amp/access_control.rs (3)

1-17: Test imports and environment setup
The rstest usage and broad set of imports are appropriate for end-to-end testing. This file sets up a robust environment using essential imports without unnecessary overhead.


26-87: Fixture function creates a clear and flexible test environment
Establishing wallets, deploying mock CW721 and app-contract, and setting up permissioning is neatly handled. The approach is coherent and should simplify future expansions of these tests.


88-129: Parameterized custom mint permission tests
Using #[rstest] with multiple cases is concise and effective. The test logic properly checks success/failure and verifies token ownership. Code readability is good, and the approach is straightforward to maintain.

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

106-106: Refactored permission_action call
Switching from permission_action(SUBMIT_FORM_ACTION, deps.storage) to permission_action(deps.storage, SUBMIT_FORM_ACTION) aligns with the updated function signature.

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

55-55: Use of deps.storage as first argument looks correct.
This change aligns with the refactoring efforts across the codebase to consistently pass deps.storage as the first argument to permission_action.

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

85-85: Constructor-based ExecuteContext instantiation improves clarity.
The use of ExecuteContext::new(deps, info, env) encapsulates the initialization logic in a single place, making the code easier to maintain.

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

53-54: Consistent usage of permission_action with deps.storage.
Passing deps.storage as the first parameter maintains consistency with the new function signature used across the codebase and helps prevent future maintenance issues.

packages/andromeda-finance/src/splitter.rs (5)

61-62: New attributes for UpdateRecipients enhance clarity.
Marking this variant as restricted, nonpayable, and direct strongly indicates the restrictions.


64-65: UpdateLock correctly marked as restricted, nonpayable, and direct.
These annotations clearly convey the operational and permission constraints, improving maintainability.


67-68: UpdateDefaultRecipient variant attributes strengthen security.
This ensures only authorized calls can update the default recipient and that it is nonpayable.


69-69: Receive is now declared nonpayable.
This annotation is a welcome restriction, preventing unintended fund transfers when invoking Receive(Cw20ReceiveMsg).


72-72: Send formatting changes look consistent.
No functional changes appear in this variant other than reformatting, which maintains overall code consistency.

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

52-53: Use of updated permission_action call signature looks correct.

The new parameter order (passing deps.storage first) is consistent with the rest of the codebase. Ensure all calls to permission_action are aligned in your tests and other contracts to avoid signature mismatches.

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

133-133: Good consistency update for permission_action parameter order.

Aligning the parameter order here is consistent with recent signature changes. No issues identified in logic.

e2e-tests/tests/macro_tests/exec_fn.rs (4)

29-32: Execution method structure is minimal but clear.

The execute function currently returns an empty Response. If specialized logic is planned in the future, ensure you maintain attribute-based checks consistently here.


59-102: Structured tests are thorough and handle multiple combinations.

Your approach systematically checks each variant with and without funds. The usage of rstest and parameterized test cases is excellent for coverage.


108-155: AMP wrapper tests effectively mirror direct calls.

By verifying both “no funds” and “with funds” scenarios, you ensure that the combined AMP path respects attribute constraints.


157-175: Unwrapping AMP messages in tests is validated carefully.

This test case confirms the correct extraction of the underlying message and context from an AMP packet. It's crucial for preventing mismatch in debugging scenarios.

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

138-145: ExecuteContext::new usage clarifies instantiation logic.

Properly constructing the ExecuteContext helps ensure your environment, message info, and branching of deps are all handled in a consistent manner. No issues spotted with the updated approach.

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

67-67: Ensure consistent usage of the reordered permission_action parameters.

The parameter order has changed to (deps.storage, SEND_CW20_ACTION). This looks correct here, but verify that all other permission_action calls in the codebase have also been updated accordingly to avoid confusion and potential runtime errors.

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

530-537: Confirm that MessageInfo fields accurately reflect the restaking state.

Overriding sender and funds with the values from restaking_queue aligns with the contract’s logic to restake rewards on behalf of the original delegator. This approach looks consistent, but ensure that no additional permission checks are necessary when simulating a transaction from an alternate sender.

packages/std/src/error.rs (1)

5-5: Public re-exports facilitate broader usage in external crates.

Re-exporting ParseReplyError, PaymentError, and ThresholdError is beneficial for downstream code that needs to leverage these error types.

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

73-73: Good use of permissioning for CW20 actions.
This call to permission_action(deps.storage, SEND_CW20_ACTION) helps ensure only authorized operations can send CW20 tokens within the crowdfund contract.


609-610: Verify the origin logic in AMPPkt.
Setting the auction's origin to the contract owner by calling .with_origin(owner) might impact AMP message flow. Confirm that the final message flow should indeed attribute the owner as the origin.

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

344-344: Parameter order updated in permission_action.
Reversing the parameter order to accept store first is consistent with standard practice and improves clarity.


366-366: Compatible call to the updated method signature.
The call to permission_action(ctx.deps.storage, action_string.clone()) here aligns with the revised parameter order and ensures cohesive permission checking logic.


566-566: Refined testing approach for permission_action.
Invoking permission_action(deps.as_mut().storage, action) in tests is correct and verifies the new parameter order.


609-609: Maintaining consistency in permission_action calls.
Using permission_action(deps.as_mut().storage, action) further unifies the updated method signature across the codebase.

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

311-311: Permission enforced on auction creation.
Calling permission_action(deps.storage, auction_id.to_string()) ensures that only whitelisted or permissible addresses can interact with the new auction.


434-434: Aligning auction updates with permission requirements.
Invoking permission_action(deps.storage, token_auction_state.auction_id.to_string()) maintains consistent permission checks for modifying the auction.

contracts/non-fungible-tokens/andromeda-crowdfund/src/testing/tests.rs (1)

1107-1107: Re-check origin assignment in test AMPPkt.
Switching the origin from MOCK_CONTRACT_ADDR.to_string() to MOCK_DEFAULT_OWNER.to_string() will change the context in which the AMP message is dispatched. Verify it aligns with the intended ownership flow.

packages/std/macros/Cargo.toml (2)

3-3: Version bump aligns with PR scope

The beta version increment (1.0.1-b.1) appropriately reflects the significant macro improvements while indicating the changes are still under development.


17-17: Verify syn 2.0 compatibility

The major version upgrade of the syn crate (1.0 -> 2.0) and addition of the "full" feature warrant careful testing.

✅ Verification successful

Let me search for all syn dependencies and its usage patterns to better understand the impact of the upgrade.


Let me check the specific usage patterns to ensure there are no compatibility issues with syn 2.0.


syn 2.0 upgrade appears safe to proceed

The codebase uses standard syn features that are maintained in 2.0:

  • Basic parsing with parse_macro_input! macro
  • Core types like DeriveInput, ItemFn, and DataEnum
  • No usage of deprecated or removed syn 1.x features
  • No other syn 1.x dependencies that could conflict

The upgrade should be compatible with the current usage patterns in the procedural macros.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any syn 1.x dependencies that might conflict
rg -A 1 'syn.*=.*1\.' 

Length of output: 23


Script:

#!/bin/bash
# Check for all syn dependencies in Cargo.toml files
rg -A 1 'syn.*=.*\d' 

# Check for any syn imports or usage patterns
ast-grep --pattern 'use syn::$$$'

# Look for potential syn-related proc macro code
ast-grep --pattern 'impl $_ for $_ {
    fn $_(input: TokenStream) -> TokenStream {
        $$$
    }
}'

Length of output: 365


Script:

#!/bin/bash
# Let's check the actual usage of syn in the codebase
rg -A 5 'use syn' packages/std/macros/src/

# Look for specific syn 2.0 features being used
ast-grep --pattern 'parse_macro_input!($$$)'

# Check for any parse_* functions from syn
ast-grep --pattern 'syn::parse_$$$'

Length of output: 2955

packages/std/Cargo.toml (1)

3-3: Version bump reflects incremental improvements

The beta version increment (b.3 -> b.4) appropriately reflects the ongoing development of the standard library improvements.

Cargo.toml (3)

17-17: Test directory rename improves clarity

The rename from "tests-integration" to "e2e-tests" better reflects the purpose of these tests and follows modern naming conventions.


70-71: Consider updating other dependencies

While updating dependencies, consider reviewing and potentially updating jsonschema-valid and serde_json versions for security patches.


37-37: Verify workspace-wide version compatibility

The andromeda-macros version update needs to be compatible across all workspace members.

✅ Verification successful

Let me gather more information to verify if there are any other workspace members using andromeda-macros.


Version compatibility verified - workspace inheritance in place

The version update is safe because:

  • The root Cargo.toml defines andromeda-macros with version 1.0.1-b.1
  • Other workspace members (packages/std and packages/deploy) use workspace inheritance with workspace = true
  • The package itself (packages/std/macros) is the source of the dependency
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any potential version conflicts in workspace members
fd -e toml | xargs rg 'andromeda-macros.*version.*1\.0\.'

Length of output: 173


Script:

#!/bin/bash
# First, let's check the workspace members
rg -A5 '\[workspace\]' Cargo.toml

# Then, let's check for any usage of andromeda-macros in all Cargo.toml files
fd -e toml | xargs rg 'andromeda-macros'

Length of output: 582

Makefile (1)

52-54: LGTM! Consistent renaming of integration tests to e2e tests.

The renaming from integration-test to e2e-test aligns with modern testing terminology and maintains consistency across the project.

Also applies to: 58-58

e2e-tests/Cargo.toml (1)

2-2: LGTM! Package name updated consistently.

The package renaming from "tests-integration" to "e2e-tests" maintains consistency with the Makefile changes.

packages/std/src/amp/messages.rs (1)

267-271: LGTM! Clean implementation of with_origin method.

The implementation follows Rust idioms by creating a clone and maintaining immutability.

Copy link
Contributor

coderabbitai bot commented Dec 27, 2024

✅ Actions performed

Reviews paused.

@crnbarr93
Copy link
Contributor Author

@coderabbitai resume

Copy link
Contributor

coderabbitai bot commented Dec 27, 2024

✅ Actions performed

Reviews resumed.

@github-actions github-actions bot added the packages/std Includes changes to the Standard crate label Jan 21, 2025
@crnbarr93 crnbarr93 merged commit 334874a into main Jan 22, 2025
6 of 7 checks passed
@crnbarr93 crnbarr93 deleted the connor/amp-receive-macro branch January 22, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/std Includes changes to the Standard crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants