Add macros package and the with_components macro #1282

ericnordelo · 2025-01-02T17:22:50Z

Quickstart #1258

NOTE: A summary of the features included in the macro is added below after the example:

Example:

Simplifies this:

#[starknet::contract]
pub mod MyToken {
    use openzeppelin_access::ownable::OwnableComponent;
    use openzeppelin_token::erc20::{ERC20Component, ERC20HooksEmptyImpl};
    use starknet::ContractAddress;

    component!(path: ERC20Component, storage: erc20, event: ERC20Event);
    component!(path: OwnableComponent, storage: ownable, event: OwnableEvent);

    impl ERC20InternalImpl = ERC20Component::InternalImpl<ContractState>;
    impl OwnableInternalImpl = OwnableComponent::InternalImpl<ContractState>;

    #[storage]
    pub struct Storage {
        #[substorage(v0)]
        erc20: ERC20Component::Storage,
        #[substorage(v0)]
        ownable: OwnableComponent::Storage,
    }

    #[event]
    #[derive(Drop, starknet::Event)]
    enum Event {
        #[flat]
        ERC20Event: ERC20Component::Event,
        #[flat]
        OwnableEvent: OwnableComponent::Event,
    }

    #[constructor]
    fn constructor(ref self: ContractState, owner: ContractAddress) {
        self.ownable.initializer(owner);
        self.erc20.initializer("MyToken", "MTK");
    }
}

Into this:

#[with_components(ERC20, Ownable)]
#[starknet::contract]
pub mod MyToken {
    use openzeppelin_token::erc20::ERC20HooksEmptyImpl;
    use starknet::ContractAddress;

    #[storage]
    pub struct Storage {}

    #[constructor]
    fn constructor(ref self: ContractState, owner: ContractAddress) {
        self.ownable.initializer(owner);
        self.erc20.initializer("MyToken", "MTK");
    }
}

Summary of features:

IT DOES:

Accepts multiple components from an internal whitelist (currently only ERC20 and Ownable for the POC).
Validates that the contract has the #[startknet::contract] attribute, shows an error in compilation otherwise.
Validates that the contract has the corresponding initializers in the constructor, shows a warning listing the missing initializers otherwise.
Adds the use clauses for each component (i.e: use openzeppelin_access::ownable::OwnableComponent;)
Adds the corresponding component! macros (i.e: component!(path: OwnableComponent, storage: ownable, event: OwnableEvent);).
Adds the corresponding internal implementations (i.e: impl OwnableInternalImpl = OwnableComponent::InternalImpl<ContractState>;)
Adds the storage entries to the Storage struct, using the lowercase version of the component name without the component keyword (i.e: ownable: OwnableComponent::Storage,)
Adds the event entries to the Event enum, and creates the enum if it is missing.

IT DOES NOT:

Adds the external implementations or hooks.
Create the constructor and/or initializers.

Things to be considered still:

openzeppelin_access vs openzeppelin::access in use clauses (configurable notation?)
Configurable storage and event entries per component.
Add default external embedded impls?
Add warnings to other things, like missing camelCase interface.

Some may be addressed in different PRs/issues.

NOTE: I have plans to include a with_component macro which will accept only a single component but more configuration, like replacing the storage entry. People then may use both macros together (with_components and with_component)

PR Checklist

Tests
Documentation
Added entry to CHANGELOG.md
Tried the feature on a public network

…eat/add-macros-package

andrew-fleming

Fantastic work on this @ericnordelo! I left some comments

Also, since so much of the component boilerplate is abstracted away, I don't think it's super obvious what the storage entry is for a given component. It'll be documented, for sure, but would it make sense just to simplify it further and use the same capitalized name as the whitelisted component i.e.

#[with_components(ERC20, Ownable)]
#[starknet::contract]
pub mod MyToken {
    (...)

    #[constructor]
    fn constructor(ref self: ContractState, owner: ContractAddress) {
        self.Ownable.initializer(owner);
        self.ERC20.initializer("MyToken", "MTK");
    }
}

This gives a point of reference inside the contract as opposed to attempting, for example, reentrancy_guard or reentrancyguard

docs/modules/ROOT/pages/api/macros.adoc

...ts/openzeppelin_macros__tests__test_with_components__with_access_control_no_initializer.snap

packages/macros/src/tests/test_with_components.rs

packages/macros/src/with_components.rs

packages/macros/src/tests/test_with_components.rs

...snapshots/openzeppelin_macros__tests__test_with_components__with_account_no_initializer.snap

.../openzeppelin_macros__tests__test_with_components__with_erc1155_receiver_no_initializer.snap

packages/macros/src/with_components.rs

andrew-fleming · 2025-01-29T01:36:51Z

packages/macros/src/with_components.rs

+        "AccessControl" => Some(ComponentInfo {
+            name: "AccessControlComponent".to_string(),
+            path: "openzeppelin_access::accesscontrol::AccessControlComponent".to_string(),
+            storage: "accesscontrol".to_string(),


Same here. I know the lib isn't entirely consistent (might be worth addressing in a separate issue), but I think it could get weird when we have timelock_controller vs accesscontrol and reentrancyguard

I will refactor them here (for macros). We may update presets and mocks in a separate issue later.

Co-authored-by: Andrew Fleming <[email protected]>

ericnordelo · 2025-01-29T12:09:05Z

This gives a point of reference inside the contract as opposed to attempting, for example, reentrancy_guard or reentrancyguard

Since developers are already using snake case for storage members, I vote we stay with snake case, but split the words accordingly.

andrew-fleming · 2025-01-29T16:14:57Z

This gives a point of reference inside the contract as opposed to attempting, for example, reentrancy_guard or reentrancyguard

Since developers are already using snake case for storage members, I vote we stay with snake case, but split the words accordingly.

This is true...no strong pushback against keeping snake case, I just think reusing the name could be a bit more intuitive. If we wanted to make a change like this, it'd be better to do it now with the feature

immrsd

Well done, Eric, great job!

I left several minor suggestions of how we could improve the code structure

immrsd · 2025-02-07T08:10:08Z

packages/macros/src/with_components.rs

+        if component_info.is_none() {
+            return ProcMacroResult::new(TokenStream::empty()).with_diagnostics(diagnostics);
+        }
+        components_info.push(component_info.unwrap());


To avoid unsafe unwrapping when possible:

Suggested change

if component_info.is_none() {

return ProcMacroResult::new(TokenStream::empty()).with_diagnostics(diagnostics);

}

components_info.push(component_info.unwrap());

if let Some(info) = component_info {

components_info.push(info);

} else {

return ProcMacroResult::new(TokenStream::empty()).with_diagnostics(diagnostics);

}

immrsd · 2025-02-07T08:20:54Z

packages/macros/src/with_components.rs

+    let parsed = db.parse_virtual(item_stream.to_string());
+    if parsed.is_err() {
+        let error_message = parsed.err().unwrap().format(&db);
+        let error = Diagnostic::error(error_message);
+        return ProcMacroResult::new(TokenStream::empty()).with_diagnostics(error.into());
+    }
+
+    // 3. Build the patch
+    let node = parsed.unwrap();
+    let (content, mut diagnostics) = build_patch(&db, node, components_info.clone());


Suggested change

let parsed = db.parse_virtual(item_stream.to_string());

if parsed.is_err() {

let error_message = parsed.err().unwrap().format(&db);

let error = Diagnostic::error(error_message);

return ProcMacroResult::new(TokenStream::empty()).with_diagnostics(error.into());

}

// 3. Build the patch

let node = parsed.unwrap();

let (content, mut diagnostics) = build_patch(&db, node, components_info.clone());

let (content, mut diagnostics) = match db.parse_virtual(item_stream.to_string()) {

Ok(node) => build_patch(&db, node, components_info.clone()),

Err(err) => {

let error = Diagnostic::error(err.format(&db));

return ProcMacroResult::new(TokenStream::empty()).with_diagnostics(error.into());

}

}

immrsd · 2025-02-07T08:32:47Z

packages/macros/src/with_components.rs

+                }
+            }
+        }
+    }


Should we add an else clause with an error?

if let RewriteNode::Copied(copied) = node { ... } else { // panic with an error }

immrsd · 2025-02-07T08:41:28Z

packages/macros/src/with_components.rs

+            let components_with_initializer_missing_str =
+                components_with_initializer_missing.join(", ");
+            if !components_with_initializer_missing.is_empty() {
+                let warning = Diagnostic::warn(formatdoc! {"
+                    It looks like the initializers for the following components are missing:
+
+                    {components_with_initializer_missing_str}
+
+                    This may lead to unexpected behavior. We recommend adding the corresponding initializer calls to the constructor.
+                "});
+                warnings.push(warning);
+            }


We can move the line building the components_with_initializer_missing_str into the if clause

Suggested change

let components_with_initializer_missing_str =

components_with_initializer_missing.join(", ");

if !components_with_initializer_missing.is_empty() {

let warning = Diagnostic::warn(formatdoc! {"

It looks like the initializers for the following components are missing:

{components_with_initializer_missing_str}

This may lead to unexpected behavior. We recommend adding the corresponding initializer calls to the constructor.

"});

warnings.push(warning);

}

if !components_with_initializer_missing.is_empty() {

let components_with_initializer_missing_str = components_with_initializer_missing.join(", ");

let warning = Diagnostic::warn(formatdoc! {"

It looks like the initializers for the following components are missing:

{components_with_initializer_missing_str}

This may lead to unexpected behavior. We recommend adding the corresponding initializer calls to the constructor.

"});

warnings.push(warning);

}

immrsd · 2025-02-07T08:45:10Z

packages/macros/src/with_components.rs

+            let mut constructor_code = String::new();
+            let constructor = body.items_vec(db).into_iter().find(|item| {
+                matches!(item, ast::ModuleItem::FreeFunction(function_ast) if function_ast.has_attr(db, CONSTRUCTOR_ATTRIBUTE))
+            });
+            if constructor.is_some() {
+                // Get the constructor code (maybe we can do this without the builder)
+                let constructor_ast = constructor.unwrap().as_syntax_node();
+                let typed = ast::ModuleItem::from_syntax_node(db, constructor_ast.clone());
+                let constructor_rnode = RewriteNode::from_ast(&typed);
+
+                let mut builder = PatchBuilder::new_ex(db, &constructor_ast);
+                builder.add_modified(constructor_rnode);
+                (constructor_code, _) = builder.build();
+            }


To avoid unsafe unwrapping and to make the constructor_code var immutable:

Suggested change

let mut constructor_code = String::new();

let constructor = body.items_vec(db).into_iter().find(|item| {

matches!(item, ast::ModuleItem::FreeFunction(function_ast) if function_ast.has_attr(db, CONSTRUCTOR_ATTRIBUTE))

});

if constructor.is_some() {

// Get the constructor code (maybe we can do this without the builder)

let constructor_ast = constructor.unwrap().as_syntax_node();

let typed = ast::ModuleItem::from_syntax_node(db, constructor_ast.clone());

let constructor_rnode = RewriteNode::from_ast(&typed);

let mut builder = PatchBuilder::new_ex(db, &constructor_ast);

builder.add_modified(constructor_rnode);

(constructor_code, _) = builder.build();

}

let constructor = body.items_vec(db).into_iter().find(|item| {

matches!(item, ast::ModuleItem::FreeFunction(function_ast) if function_ast.has_attr(db, CONSTRUCTOR_ATTRIBUTE))

});

let constructor_code = if let Some(constructor) = constructor {

// Get the constructor code (maybe we can do this without the builder)

let constructor_ast = constructor.as_syntax_node();

let typed = ast::ModuleItem::from_syntax_node(db, constructor_ast.clone());

let constructor_rnode = RewriteNode::from_ast(&typed);

let mut builder = PatchBuilder::new_ex(db, &constructor_ast);

builder.add_modified(constructor_rnode);

(constructor_code, _) = builder.build();

} else {

String::new()

};

immrsd · 2025-02-07T08:49:03Z

packages/macros/src/with_components.rs

+            let error = Diagnostic::error(formatdoc! {"
+                Contract module must have the `#[{CONTRACT_ATTRIBUTE}]` attribute.
+            "});
+            return (vec![error], vec![]);


Suggested change

return (vec![error], vec![]);

return (vec![error], warnings);

immrsd · 2025-02-07T08:56:03Z

packages/macros/src/with_components.rs

+/// Allowed components are:
+/// `ERC20`, `Ownable`


We should update the list of the allowed components:

Suggested change

/// Allowed components are:

/// `ERC20`, `Ownable`

/// Allowed components are:

/// `ERC20`, `Ownable`

immrsd · 2025-02-07T08:59:49Z

packages/macros/src/with_components.rs

+///
+/// Allowed components are:
+/// `ERC20`, `Ownable`
+fn get_component_info(name: &str) -> (Option<ComponentInfo>, Diagnostics) {


I guess it's better to change the function's output type to Result<ComponentInfo, Diagnostics>. It would make both the function itself and the code calling it more clear

Suggested change

fn get_component_info(name: &str) -> (Option<ComponentInfo>, Diagnostics) {

fn get_component_info(name: &str) -> Result<ComponentInfo, Diagnostics> {

immrsd · 2025-02-07T09:16:29Z

packages/macros/src/with_components.rs

+pub struct ComponentInfo {
+    pub name: String,
+    pub path: String,
+    pub storage: String,
+    pub event: String,
+    pub has_initializer: bool,
+    pub has_immutable_config: bool,
+    pub internal_impls: Vec<String>,
+}


It looks like we don't need owned string in the code, so we could use &str type instead of String to make code cleaner and more performant. Especially the get_component_info will benefit from it if we can get rid of all the to_string() calls

Suggested change

pub struct ComponentInfo {

pub name: String,

pub path: String,

pub storage: String,

pub event: String,

pub has_initializer: bool,

pub has_immutable_config: bool,

pub internal_impls: Vec<String>,

}

pub struct ComponentInfo {

pub name: &str,

pub path: &str,

pub storage: &str,

pub event: &str,

pub has_initializer: bool,

pub has_immutable_config: bool,

pub internal_impls: Vec<&str>,

}

immrsd · 2025-02-07T09:18:51Z

packages/macros/src/with_components.rs

+            internal_impls: vec!["InternalImpl".to_string()],
+        }),
+        _ => None,
+    };
+    if info.is_none() {
+        let allowed_components = ALLOWED_COMPONENTS.join(", ");
+        let error_message = formatdoc! {"
+            Invalid component: {name}
+
+            Allowed components are:
+            {allowed_components}
+        "};
+        let error = Diagnostic::error(error_message);
+        return (None, error.into());
+    }


Suggested change

internal_impls: vec!["InternalImpl".to_string()],

}),

_ => None,

};

if info.is_none() {

let allowed_components = ALLOWED_COMPONENTS.join(", ");

let error_message = formatdoc! {"

Invalid component: {name}

Allowed components are:

{allowed_components}

"};

let error = Diagnostic::error(error_message);

return (None, error.into());

}

internal_impls: vec!["InternalImpl".to_string()],

}),

_ => {

let allowed_components = ALLOWED_COMPONENTS.join(", ");

let error_message = formatdoc! {"

Invalid component: {name}

Allowed components are:

{allowed_components}

"};

let error = Diagnostic::error(error_message);

Err(error.into())

}

}

ericnordelo added 6 commits December 16, 2024 13:47

feat: bump scarb

fde5105

Merge branch 'feat/bump-scarb-to-2.9.2' into feat/add-macros-package

e311c2f

feat: add crate

89d9ea4

feat: add oz_component file

5bebd2d

feat: add main logic

b75027f

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…

c164f00

…eat/add-macros-package

ericnordelo requested review from andrew-fleming, immrsd and ggonzalez94 January 2, 2025 17:29

ericnordelo linked an issue Jan 6, 2025 that may be closed by this pull request

Provide macros as part of the library #1258

Open

ericnordelo added 13 commits January 8, 2025 14:12

feat: add snapshot tests to macros

acd4516

feat: format output

6814d0b

feat: add warnings

44c6491

feat: add tests for new warnings

72fdfeb

feat: add snapshots

e81beec

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…

5762562

…eat/add-macros-package

feat: add more tests

06705c6

feat: add more tests and warnings

d0d9faf

feat: remove unused dependency

ba61a95

feat: add github action

81b9997

feat: add doc entry

0a2d5de

fix: typo

b2f67e9

feat: add accounts

02daa77

ericnordelo marked this pull request as ready for review January 22, 2025 14:10

ericnordelo added 4 commits January 22, 2025 15:11

feat: update CHANGELOG

edb6688

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…

904b7f8

…eat/add-macros-package

feat: test existing event struct

c58bae0

docs: finish with_components entry

c8e802d

andrew-fleming reviewed Jan 29, 2025

View reviewed changes

Update docs/modules/ROOT/pages/api/macros.adoc

e1e3db8

Co-authored-by: Andrew Fleming <[email protected]>

ericnordelo and others added 4 commits January 29, 2025 13:02

Update packages/macros/src/tests/test_with_components.rs

8d25611

Co-authored-by: Andrew Fleming <[email protected]>

Update packages/macros/src/tests/test_with_components.rs

32754b9

Co-authored-by: Andrew Fleming <[email protected]>

Update packages/macros/src/with_components.rs

6036d83

Co-authored-by: Andrew Fleming <[email protected]>

Update packages/macros/src/tests/test_with_components.rs

455cc40

Co-authored-by: Andrew Fleming <[email protected]>

feat: apply review updates

3c757b7

immrsd reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add macros package and the with_components macro #1282

Add macros package and the with_components macro #1282

ericnordelo commented Jan 2, 2025 •

edited

Loading

andrew-fleming left a comment

andrew-fleming Jan 29, 2025

ericnordelo Jan 29, 2025

ericnordelo commented Jan 29, 2025

andrew-fleming commented Jan 29, 2025

immrsd left a comment

immrsd Feb 7, 2025

immrsd Feb 7, 2025

immrsd Feb 7, 2025

immrsd Feb 7, 2025

immrsd Feb 7, 2025

immrsd Feb 7, 2025

immrsd Feb 7, 2025

immrsd Feb 7, 2025

immrsd Feb 7, 2025

immrsd Feb 7, 2025

	return (vec![error], vec![]);
	return (vec![error], warnings);

	fn get_component_info(name: &str) -> (Option<ComponentInfo>, Diagnostics) {
	fn get_component_info(name: &str) -> Result<ComponentInfo, Diagnostics> {

Add macros package and the with_components macro #1282

Are you sure you want to change the base?

Add macros package and the with_components macro #1282

Conversation

ericnordelo commented Jan 2, 2025 • edited Loading

PR Checklist

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo commented Jan 29, 2025

andrew-fleming commented Jan 29, 2025

immrsd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo commented Jan 2, 2025 •

edited

Loading