feat: WIP GNO mail (no render) #641
Conversation
|
Thank you for taking the time to contribute 🙏 . I left some initial review comments. Will do a detailed review later. Please consider running |
|
Thank you for these initial feedbacks Harry 🙏 . Commited the suggested changes and made sure test passed. |
| import ( | ||
| "gno.land/p/demo/avl" | ||
| "gno.land/p/demo/releases" | ||
| "std" | ||
| ) | ||
|
|
||
| // realm state | ||
| var ( | ||
| addr2Mailbox = avl.Tree{} // Address -> *Mailbox | ||
| counter int = 0 // number of mails ever sent | ||
| admin std.Address = "g1fjh9y7ausp27dqsdq0qrcsnmgvwm6829v2au7d" // @grepsuzette | ||
|
|
||
| Fee std.Coin = std.Coin{ | ||
| Denom: "ugnot", | ||
| Amount: 250 * 1000, // should never cost more $0.12 or 0.25 | ||
| } | ||
| ) |
There was a problem hiding this comment.
I don't think we should be separating out state from the base file where that state is actively being used - it just creates confusion on where it's coming from. What do you think about moving this snippet closer to where it's being used?
There was a problem hiding this comment.
I certainly lack experience here, I see Thomas agrees with you.
I'll let you know my thought to have a chance to understand why you disagree. For me:
- State is the most central thing for a realm.
- The code gravitates around it and is interchangeable.
- Therefore knowing what is state is key to approaching a realm.
Approaching one realm, I always look for the part where the state is defined.
The most obvious label for me for the state is to look for a file called state.gno.
This way it's possible to open state.gno in one split and work on something else.
If a folder doesn't contain one, I will respect the author choice, but then I will have to open a lot of files until I find it.
But in fact, I don't even argue to have files called a certain way, I argue authors should have their own style; I think diversity is good for the project, like in evolution. We don't really know what is best at this point IMO.
| ) | ||
|
|
||
| type Mail struct { | ||
| id int |
There was a problem hiding this comment.
I'm not sure we should be using an int for a Mail ID - considering this can be something that is potentially used a lot as a message passing realm. Message IDs should probably be something derived from the mail message itself, rather than a counter
There was a problem hiding this comment.
Me neither.
But if int in gno means int64, and since 2^63 is 9,223,372,036,854,775,808.
Let's say the system is so popular it generates one million messages per seconds.
It would take 92233720368 seconds to overflow, that is 2924 years.
While it's not perfect (one source claims more than 3 millions email were sent per seconds in 2023) I think this is a bigger discussion.
- hash(msg): not good IMO (collisions will be trivial to find in the future),
- the transaction number that produced the message may seem good, except when the system can send a message to several recipients, it will need a suffix to work.
As it's WIP and we have staging testnets I think we can keep an int for the time being.
There was a problem hiding this comment.
Let's use int for now and later switch to bigint (#764), maybe.
| @@ -0,0 +1,42 @@ | |||
| package mail | |||
There was a problem hiding this comment.
What do you think about moving this Mail library logic to examples/gno.land/p/demo/mail, instead of having it sit in a Realm?
There was a problem hiding this comment.
Hey Milos!
Thanks taking the time to review this. Yes, deciding what to put in p/ and r/ has been one interrogation of mine.
Here are my thoughts, here is my starting point:
pis package, is like library, is common code for everyone to use, meaning it's quite stable.ris realm, is app-like, meaning usage is restricted.
By that logic, code that isn't reasonnably finalized at least during GoR is better kept in r/ as it won't break anything (because nobody will be using it as a dependency).
I think putting things in p/demo is a signal to other devs saying: this code is stable, well-thought out please use it. My app is not there yet, it's WIP.
Another question I have with this app is:
Whether the functionality it offers is really about mails? or about generic messages.
If it's the latter, then the part to put in p/ is not mail, it would be like p/demo/msg, with r/demo/mail being a realm (almost an app instance) using that system.
This tells me it's a bit early to make the separation now.
|
tests don't pass, please wait. |
|
Hey @grepsuzette ! Do you need any help to move this mail realm forward? Thanks! |
|
Closing as draft for 1yr+. You're welcome to re-open to continue it. |
A mail system for GNO, that can be added to demo/ for now.
The low-level is functional, but there is no UI (Render() is "TODO").
Contributions welcome :)
Also as you can see in the notes, I have my doubt if this shouldn't be a more generic system.
Yep. Let's go!