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

Idiomatic Rust should be a priority for OpenBrush DSL #121

Open
kvinwang opened this issue Jul 25, 2023 · 0 comments
Open

Idiomatic Rust should be a priority for OpenBrush DSL #121

kvinwang opened this issue Jul 25, 2023 · 0 comments
Assignees
Labels
enhancement New feature or request

Comments

@kvinwang
Copy link

I have been working with ink! contracts and recently took a look at the latest version of OpenBrush. I noticed that the examples provided in OpenBrush appear to diverge from conventional Rust syntax and intuition, which can be unsettling for those familiar with the language. For the DSL to be widely accepted and user-friendly, I believe it should resemble idiomatic Rust as closely as possible.

To illustrate, let's use the 'Ownable' example. Here's the relevant code:

#![cfg_attr(not(feature = "std"), no_std, no_main)]

#[openbrush::implementation(Ownable, PSP37, PSP37Burnable, PSP37Mintable)]
#[openbrush::contract]
pub mod ownable {
    use openbrush::{
        modifiers,
        traits::Storage,
    };

    #[ink(storage)]
    #[derive(Default, Storage)]
    pub struct Contract {
        #[storage_field]
        psp37: psp37::Data,
        #[storage_field]
        ownable: ownable::Data,
    }

    impl Contract {
        #[ink(constructor)]
        pub fn new() -> Self {
            let mut instance = Self::default();
            ownable::Internal::_init_with_owner(&mut instance, Self::env().caller());
            instance
        }
    }

    #[default_impl(PSP37Mintable)]
    #[modifiers(only_owner)]
    fn mint(&mut self) {}

    #[default_impl(PSP37Burnable)]
    #[modifiers(only_owner)]
    fn burn(&mut self) {}
}

There are several aspects of this example that seem to contravene Rust conventions and visual intuition:

  1. Trait Usage Without Prior Import:

    #[openbrush::implementation(Ownable, PSP37, PSP37Burnable, PSP37Mintable)]

    In this example, Ownable, PSP37,... appear abruptly, without a preceding use statement. The traits should be imported with use first.

  2. Positioning of #[openbrush::implementation(Ownable, PSP37, PSP37Burnable, PSP37Mintable)]
    Why is this line placed above mod ownable? Intuitively, it seems it should be applied to struct Contract, as it says impl Ownable,PSP37, PSP37Burnable, PSP37Mintable for Contract, doesn't it?

  3. Explicit Declaration of Storage Fields Required by Trait Implementation
    The way the storage field ownable is implicitly bound to the trait implementation is not immediately clear. I believe the relationship between the trait implementation and the storage field should be visually explicit, something like:

    #[openbrush::implementation(Ownable(storage=ownable),...)]
  4. Use of Leading _ for Internal Methods

    ownable::Internal::_init_with_owner(&mut instance, Self::env().caller());

    In Rust, a leading _ signals to the compiler to suppress unused item warnings. Overloading it with additional meaning can have unforeseen side effects. Moreover, the name Internal is a bit of a misnomer, and it may be worthwhile to consider a more descriptive name.

  5. default_impl(Trait)

    #[default_impl(PSP37Mintable)]
    #[modifiers(only_owner)]
    fn mint(&mut self) {}

    This syntax is quite confusing, as fn mint(&mut self) {} outside an impl block is not a valid Rust. A more conventional approach might be:

    #[openbrush::default_impl]
    impl PSP37Mintable for Contract {
       #[modifiers(only_owner)]
       fn mint(&mut self) {}
    }
@Artemka374 Artemka374 self-assigned this Jul 26, 2023
@Artemka374 Artemka374 added the enhancement New feature or request label Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants