feat: Adding a lottery to examples #1850
Conversation
github-actions
bot
added
the
🧾 package/realm
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1850 +/- ##
==========================================
+ Coverage 63.30% 63.43% +0.12%
==========================================
Files 548 535 -13
Lines 78511 84991 +6480
==========================================
+ Hits 49704 53916 +4212
- Misses 25452 27548 +2096
- Partials 3355 3527 +172
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for the contribution and tutorial!
I've left comments on which parts can be improved. I suggest that for each conversation, you make a change, and then commit it. This way, reviewers can exactly see where you changed code to match their suggestion. You can copy commit links and paste them into the conversations. Please ping me on Signal when you finish implementing the feedback 🙏
…into add_kazai_gnolotto
|
The tidy CI has failed; please consider running the following commands: |
There was a problem hiding this comment.
Hey, I left a second round of reviews. Thanks for working on this.
Now for the hardest part - please try to write tests for the package and the realm. This will make sure that the realm is working correctly, and you will most likely discover some issues while writing the tests.
| l, _ := lotteryRaw.(*gnolotto.Lottery) | ||
|
|
||
| if time.Now().Unix() > l.DrawTime.Unix() { | ||
| panic("This lottery has already ended") | ||
| } | ||
|
|
||
| if len(numbers) != gnolotto.MaxLottoNumbers { | ||
| panic("You must select exactly 5 numbers") | ||
| } |
There was a problem hiding this comment.
Since we want to be careful with how much computation we do in Gno, I suggest you move these simple checks before more complex ones, since there is a chance to exit the transaction early and spend less gas that way.
For example, why would you check if the numbers are unique (costs more) before checking if the number of numbers is correct (costs less)?
|
Hey @kazai777, have you had a chance to look at the comments? |
Hi @zivkovicmilos , I hadn't seen the comments, I'll have a look :) |
Kouteki
added
the
review/triage-pending
jefft0
added
review/triage-pending
|
hey @leohhhn, can you take another look? |
There was a problem hiding this comment.
Hey, I left some more comments. Please, do not resolve conversations if you haven't implemented the requested change (ie, this comment). Also, the tests are failing.
| func Draw(lotteryIDStr string) string { | ||
| lotteryID, _ := seqid.FromString(lotteryIDStr) | ||
| id := lotteryID.Binary() | ||
|
|
||
| if std.PrevRealm().Addr() != admin { | ||
| panic("Only the admin can draw the winning numbers") | ||
| } |
There was a problem hiding this comment.
Exit early if the caller is not the admin; save some gas
| } | ||
|
|
||
| lotteryRaw, exists := lotteries.Get(id) | ||
|
|
|
|
||
| const MaxLottoNumbers = 5 | ||
|
|
||
| // Adds a new ticket to the lottery |
There was a problem hiding this comment.
Can you follow godoc? Start the comment with the func name. Applies for all comments
| @@ -0,0 +1,113 @@ | |||
| package gnolotto | |||
There was a problem hiding this comment.
Can you add a short godoc comment about the package to give a brief intro as to what it's about, what it can be used for?
| sentCoins := std.GetOrigSend() | ||
| amount := sentCoins.AmountOf("ugnot") | ||
|
|
||
| if prizePool != amount { | ||
| panic("Prize pool must match the transaction value") | ||
| } | ||
|
|
||
| if drawTime < time.Now().Unix() { | ||
| panic("Invalid draw time") | ||
| } | ||
|
|
||
| if std.PrevRealm().Addr() != admin { | ||
| panic("Only the admin can create a lottery") | ||
| } | ||
|
|
There was a problem hiding this comment.
First do checks, then everything else. Check for admin first.
| ) | ||
|
|
||
| // Replace this address with your address | ||
| var admin std.Address = "g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5" |
| func init() { | ||
| lotteries = avl.NewTree() | ||
| } |
There was a problem hiding this comment.
Let's initialize this tree at the declaration, outside of init.
|
|
||
| lottery := gnolotto.NewLottery(time.Unix(drawTime, 0), prizePool) | ||
|
|
||
| lotteries.Set(lotteryID.Binary(), lottery) |
There was a problem hiding this comment.
Why .Binary()? Use .String()
| func (l *Lottery) Draw() { | ||
| var blockHeight int64 = std.GetHeight() | ||
|
|
||
| l.WinningNumbers = nil | ||
| numbersMap := make(map[int]bool) | ||
|
|
||
| // Add variability to the pseudo-random number generation | ||
| var variabilityFactor int64 = 1 | ||
|
|
||
| for len(l.WinningNumbers) < MaxLottoNumbers { | ||
| simpleSeed := (blockHeight + variabilityFactor*251) % 233280 | ||
| number := int(simpleSeed%15) + 1 // Ensure number is between 1 and 15 | ||
|
|
||
| if !numbersMap[number] { | ||
| l.WinningNumbers = append(l.WinningNumbers, number) | ||
| numbersMap[number] = true | ||
| } | ||
|
|
||
| variabilityFactor += 13 // Adjusts for increased variability | ||
| } | ||
| } |
There was a problem hiding this comment.
Now that we have p/demo/entropy, let's use that. The main reason someone might check this package out in the examples folder is to check how randomness is done (lottery = random).
| return numbers, nil | ||
| } | ||
|
|
||
| func AreNumberMatching(ticketNumbers, winningNumbers []int) bool { |
There was a problem hiding this comment.
This can be a private func
| func AreNumberMatching(ticketNumbers, winningNumbers []int) bool { | |
| func areNumberMatching(ticketNumbers, winningNumbers []int) bool { |
|
Hello @kazai777 , do you have response to the latest comments? |
Adding a
gnolottolottery to the examplesr/demo/gnolotto_factoryandp/demo/gnolotto, with a README as tutorial in the realmContributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description