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

"ink::trait_definition" may not design completely for many case. #683

Open
atenjin opened this issue Feb 6, 2021 · 4 comments
Open

"ink::trait_definition" may not design completely for many case. #683

atenjin opened this issue Feb 6, 2021 · 4 comments
Labels
B-design Designing a new component, interface or functionality. B-feature-request A request for a new feature. C-discussion An issue for discussion for a given topic.

Comments

@atenjin
Copy link

atenjin commented Feb 6, 2021

Questions
"ink::trait_definition" may not design completely for many case.

Description
In my view, the "ink::trait_definition" is same as the interface part in solidity, is a way to be agreed with different contracts, third parity, and the basic element for contract specification like "ERC20", "ERC721" or else.

But now, the "ink::trait_definition" just shows some case in example. When I try to use the "ink::trait_definition" in other case, meet many problems.

  1. when split the "ink::trait_definition" part to another crate, it would meet problems. for example:
    1. put a "ink::trait_definition" in an independent crate, and the "ink::trait_definition" is in the mod which is modified by ink::contract:
      This is the example link: https://github.com/patractlabs/metis/blob/master/traits/access/ownership/lib.rs
      First, I think a specification should include the "Event" part, like ERC20 contains the spec for events. For the third part using the event to mark something happen. But currently, #[ink(event)] must in the ink::contract mod.
      But I know putting the ink::contract on mod, when other contract need to use this contract, current contract crate should be used in feature ink-as-dependency. For this example: https://github.com/patractlabs/metis/blob/master/impls/token/erc20/Cargo.toml. (erc20-trait part)
      However when using ink-as-dependency, the #[ink(event)] would be ignore and could not be used in other contracts. If not using ink-as-dependency, the contract could not be compiled. I think the event should be part of specification, thus, the event should be some part of "ink::trait_definition" or could be independent with ink::contract, or desgin some "spec event" for this case.
      And Second, if the ink::contract mod do not have the part of #[ink(storage)], [ink(constructor)] and #[ink(message)], it could not be compiled. Thus I add pub struct Phantom; in abrove example just for dompilation.
      I think if I just define an interface, I should could compile "ink::trait_definition" directly, do not need other hack operation.

    2. If do not put "ink::trait_definition" in the mod which is modified by ink::contract
      This is the example link: https://github.com/patractlabs/metis/blob/master/traits/access/ownership/lib.rs
      I define the #[ink::trait_definition] in the lib.rs directly. It would meet two problem:

      1. This crate could only be compiled when using cargo +nighlty build command, and could not be compiled with cargo +nighlty contract build. If ink! chooses cargo-contract as the tool to compile contract, I think the cargo-contract should be able to use in this case, either provides another subcommand like cargo +nighlty contract trait build, or supports #[ink::trait_definition] could be compiled without [ink(constructor)] directly.
      2. If without [ink(constructor)], current crate could not write #[ink(event)], for the event must be in the mod ink::contract now.
    3. The message which in the "ink::trait_definition" is strongly correlated with the Trait name, which would cause some problem:

      1. The "ink::trait_definition" could not do type define like: use Trait as SomeOtherName.
        e.g. If I define a Trait Erc20 in a crate and in another crate, I want to implement the Erc20 Trait for a struct Erc20, then in case the conflicted name, I try to do
        use erc20_trait::erc20::Erc20 as Erc20Trait;
        However the selector is calculated by TraitName,MessageName, thus the source selector is Erc20,transfer, and in the implementation part, it's Erc20Trait,transfer, so it cause the compilation error.
      2. I notice the "name" for "messages"/"constructors" is an array in metadata, not a value.
        I think this design is related to Trait system which just in rust. Like If someone define a trait "IErc20" which contains a message "transfer" aand define another trait "NeedTransfer" which contains same name message "transfer" too. And the two traits are implemented for one contract, then the selector should belong to different trait. So that the name is designed in an array.
        But the array is not friendly for 3rd parity tool, like just for polkadot.js's contract-api, when I call a message, just could use contract.query['erc20,balanceOf'], and could not to do the call like web3.js do, loading an abi to generate function. In Redspot, we plan to design a wrapper to change contract.query['erc20,balanceOf'] to contract.erc20.balanceOf, conventing the , to . . However in this way it's hard to add type hint for typescript. And in anther language sdk, we plan to do this operation as well.
        But all in all, I think desgin an array for this name part and generated by this name may not a good choose. I think we could design a namespace first, to express different Trait, like "IErc20" and "NeedTransfer", and this name should not be binding with the source name(to support type define like "as"). The final level for the namespace is the message function name. Thus the "name" in metadata could like:
        {
                    "name": {
                                "IErc20": "transfer"
                    }
        }
        and
        {
                    "name": {
                                "NeedTransfer": "transfer"
                    }
        }
        This modle is friendly for 3rd parity to generate the type info, and may could do some mapping for the Trait name. And if the contract define a message name same as the Trait name, it's not conflicted, for example:

        contract define a function name "IErc20":

        {
                  "name": "IErc20" 
        }
        and another trait message generates like:

        the contract is implemented by a Trait "IErc20", and this Trait provide a message "transfer"

        {
                    "name": {
                                "IErc20": "transfer"
                    }
        }
@tylerztl
Copy link
Contributor

Here is a another issue about trait definiton #737

@atenjin
Copy link
Author

atenjin commented Apr 12, 2021

event part related to #759

@xgreenx
Copy link
Collaborator

xgreenx commented Jun 8, 2021

Want to add some additional ideas=) At the moment events can only be defined inside of a contract because these are using Storage identifier during identifier generation. It Will be cool to have the ability to pass an identifier of Trait/Struct during event definition. It is needed for cases when two contracts implement the same trait. In this case, they must emit the same events, because these events were generated during the execution of this trait.

It is a critical moment for the library if we want to force everyone to use pre-defined traits to have the same behavior.

@Robbepop
Copy link
Collaborator

Robbepop commented Jun 8, 2021

We will introduce a new event syntax to ink! that will allow users to have more control over event definitions and also allows for sharing of event definitions between multiple contracts.

@HCastano HCastano added B-design Designing a new component, interface or functionality. B-feature-request A request for a new feature. C-discussion An issue for discussion for a given topic. labels Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-design Designing a new component, interface or functionality. B-feature-request A request for a new feature. C-discussion An issue for discussion for a given topic.
Projects
None yet
Development

No branches or pull requests

5 participants