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

chore: move the grc721 interface in p/grc/ #139

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

moul
Copy link
Member

@moul moul commented Apr 25, 2022

this PR does not bring real value to the NFT package itself, but intend to introduce the concept of having interfaces for common standards available from the gno.land/p/grc/* packages.

the concept becomes more interesting as soon as we have more than one, which is the case with #136 that introduces the GRC20

is it okay to go this way or better reducing the amount of imports and not a problem to mix implementation and interfaces for such standards?

@@ -5,80 +5,43 @@ import (
"strconv"

"gno.land/p/avl"
igrc721 "gno.land/p/grc/grc721"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it customary to name it "i"+name ?
I've been appending "m" instead, for "module". but I suppose not all packages are modules so...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the i prefix is common for packages that only define interfaces

to be honest, I opened this PR early with this first split, but I'm considering more and more to not only push the interface but the interface + a minimal (official?) implementation, so anyone can import it and use the implementation, or import it to ensure having compatibility with the interface

no strong conviction on the naming yet, it's up to you :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the i prefix is common for packages that only define interfaces

to be honest, I opened this PR early with this first split, but I'm considering more and more to not only push the interface but the interface + a minimal (official?) implementation, so anyone can import it and use the implementation, or import it to ensure having compatibility with the interface

no strong conviction on the naming yet, it's up to you :)

Copy link
Contributor

@jaekwon jaekwon Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's stateless, it can still live in /p/grp, and it can be known to be "pure". so as long as the package doesn't have global variables that change value, all good. NOTE if a thing tries to modify a /p/xxx variable, upon realm finalization it panics (or it should). kinda wish gno/go would support the notion of "constant" variables for this reason.

@jaekwon
Copy link
Contributor

jaekwon commented Apr 27, 2022

perfect.

@moul moul force-pushed the dev/moul/grc-if branch from 03c630b to ecd83b5 Compare April 27, 2022 07:23
@jaekwon jaekwon merged commit 699e63c into gnolang:master Apr 27, 2022
@moul moul deleted the dev/moul/grc-if branch July 5, 2022 09:16
@moul moul added this to the 🏗1️⃣ test1.gno.land milestone Oct 20, 2022
@moul moul self-assigned this Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants