-
Notifications
You must be signed in to change notification settings - Fork 13
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
Feature/minting lazy #45
Conversation
…ture/minting-lazy
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 is a great change!
I just left a couple minor doubts to to my lack of knowledge on ink!.
crates/minting/src/internal.rs
Outdated
self.get_attribute(collection_id, String::from("baseUri")) | ||
.and_then(|uri| PreludeString::from_utf8(uri).ok()) | ||
.map(|uri| uri + &token_id.to_string() + &PreludeString::from(".json")) | ||
}; |
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 very opinionated. Can implementers override it in case they want a different logic?
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.
So I left the existing logic intact but just refactored away some potentially unsafe code. Maybe @Maar-io can answer this better than me!
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.
Any of the default fn
can be overridden. Just check how Evens are handled.
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.
Do we think the from_attribute
part of the logic is clear enough? It confused me a little as it could be completely different from the metadata the user has assigned to a token, which is what the user is expecting to return.
Would it not be clearer to return None
if their metadata has not been set, and then it's up to the user to handle this with custom logic (such as what's in from_attribute
)?
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.
we do not need to use attributes at all. They come from metadata trait but if it is confusing, better to remove it and use contract's storage
Would it make sense to move |
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.
LGTM
I'll move it to ink 4.0 soon and take a deeper look
Yeah definitely, will make a separate PR for it though and keep this one to minting |
This decouples the
core
andlazy
minting functionality, and instead provides 2 traitsMinting
&MintingLazy
to allow the user to choose their desired implementation.I propose some small changes to the trait methods to allow consistency between core/lazy and provide a clearer api.
Minting (Ownable, Non-payable)
MintingLazy (Payable)
Other than minting methods, the only differences between the implementations are
Minting
havingassing_metadata
as this is not a needed for lazy, andprice
is only available inMintingLazy
.Other updates include:
Id
and(Id, Id) (id range)
for the Minting implementation for use cross-contract logic.assign_metadata
from minting logic. This provides greater flexibility. Current implementation only caters for single-mint-with-metadata.equippable-lazy
example, which is used in the JS tests.token_uri
I have also moved the minting tests into the minting crate, and split them between core & lazy.