-
Notifications
You must be signed in to change notification settings - Fork 33
Feat/contracttraits #334
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
base: main
Are you sure you want to change the base?
Feat/contracttraits #334
Conversation
✅ Deploy Preview for delightful-dieffenbachia-2097e0 canceled.
|
19c5321
to
6905c5c
Compare
Also updates macros to newest version of sdk
6905c5c
to
f29b74f
Compare
@willemneal thanks a lot for this! Introducing macros is always a tricky subject, because we are effectively creating a DSL on top of Rust, which increases the entry barrier for new adopters. Considering that, the macros introduced should have have the following properties:
For this PR, I don't think the criteria are met for the macros introduced. The reasons are:
When we weigh in the complexity introduced by the new DSL, the "magic behind curtains" introduced by the macro, harder or impossible to debug the contract related code, we are skeptic on this approach. As Leigh suggested in the discussion, if we can bring the utility of this macro without introducing a new DSL, that would be really great. However, I still think that is technically impossible due to how macros work in Rust. Note:
|
87eafed
to
2870630
Compare
Simplify Upgrade and remove migration
|
||
token_id | ||
} | ||
|
||
pub fn get_royalty_info(e: &Env, token_id: u32, sale_price: i128) -> (Address, i128) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unneeded since royalty_info
is already exposed.
This PR demonstrates how to make use the new
contracttrait
andcontract_derive
macros.default_impl
was a great first step at trying to get around the limitations of thecontract_impl
macro requiring the methods to be present preventing the ability to use default methods with a default implementation to allow for overwriting certain methods or just not requiring the boilerplate of each method in a given contract implementing a trait.Comparison with
#[default_impl]
Consider the enumerable NFT example:
The big difference is that the the contract trait macro generates all methods and redirects back to the trait's method. Also since it generates it also copies in the documentation for each method which will show up in the contract metadata and thus the CLI and generated TS.
The other big advantage is being able to override what the default implementation is. Consider
Enumerable
, withdefault_impl
, you are forced to use it. Where as withcontracttrait
you can override it withdefault =
or just in the trait implementation. It also lifts ContractType for all traits to use throughImpl
.Also since there is a separation between the implementation of the trait and the methods that get generated for the external
contract_impl
you can use&
's in the trait methods, which externally will be the value and then the referenced is passed to the contract's implementation of the trait.Furthermore it allows for methods to be marked as internal and then they won't be exposed as part of the
contractimpl
. So for example, whileinternal_mint
. can be an internal method of theFungibleToken
trait. Then any default impl will need to implement it so that you can useSelf::internal_mint
instead ofBase::mint
. It is also very helpful for different auth checks. Again allowing you to swap out the implementation and still keep the internal interface. Thus all macros which insert checks now useSelf
and no longer pin to a specific function.The biggest win though over
default_impl
is that you never need to update the macro. You change the definition of the trait and the generated code matches. Thus it is never the case that there is a mismatch between definitions making the code easier to maintain. And it makes adding new traits easy since you just write the trait.Note
I had to update the macros because there was a change in the sdk version that caused an issue. Basically the macro's got added to a constant generated for the spec. I think it should be addressed upstream.
PR Checklist