feat(examples): Gnofundme #2085
Conversation
…n getters, reshaped testfiles
…cussion about the project
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 #2085 +/- ##
==========================================
- Coverage 60.87% 60.86% -0.01%
==========================================
Files 563 563
Lines 75193 75193
==========================================
- Hits 45770 45763 -7
- Misses 26055 26062 +7
Partials 3368 3368
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| contributors []*Contributor | ||
| } | ||
|
|
||
| func NewCampaign(title string, description string, goal uint, begin time.Time, deadline time.Time, owner *User, beneficiary *User) *Campaign { |
There was a problem hiding this comment.
How will you check if the string variables aren't empty? I think it would be great if you add some conditionals to avoid this issue. Maybe a minimum of 5 chars for the title, 10 for description, a goal of more than 0, etc.
There was a problem hiding this comment.
Hey, I was wondering if I add my checks on the realm (can cost more gas) or in the front (react), is it secure to do it on the front?
There was a problem hiding this comment.
If you have a safe testing of your front, it should be ok :-)
| current: 0, | ||
| begin: begin, | ||
| deadline: deadline, | ||
| owner: owner, |
There was a problem hiding this comment.
Won't be the owner the same as the caller? If not, shouldn't we add an extra requirement to avoid people requesting funds in the name of others?
There was a problem hiding this comment.
you can see here the different types of campaigns I found on the net. So for some campaigns, someone car create a campaign for another person or organism. But I still need to figure out how do some verification on that cases.
There was a problem hiding this comment.
I agree with you about campaigns for 3p, but you should have a mechanism to avoid people to abuse about the image of others to exploit a campaign.
| users = avl.NewTree() | ||
| } | ||
|
|
||
| func NewUser(name string, email string) string { |
There was a problem hiding this comment.
What happen if the variables are empty? Also you can change the input part from
(name string, email string) to (name, email string)
since both are strings (but that's a matter of taste)
There was a problem hiding this comment.
Same thing here for verification, the other issue is as we're talking about web3, a user is supposed to be identified only by his address, but I may use profile pkg, who is a work in progress, to identify people. I'll need a bunch of people's opinions to take decision on how to handle this.
There was a problem hiding this comment.
@gfant +1
You should handle the cases where the user fields are empty :)
| "time" | ||
|
|
||
| "gno.land/p/demo/avl" | ||
| pkg "gno.land/p/demo/gnofundme" |
There was a problem hiding this comment.
I think this name pkg could be better called to avoid confusion since it's not the only package you're calling. This would help the readability of your code.
There was a problem hiding this comment.
thanks for the advise, I'll modify it before the final version.
| // the avl.tree key will be address to simplify the fetching | ||
| users.Set(caller.String(), user) | ||
|
|
||
| user = pkg.NewUser("user2", "email2", beneficiary) |
There was a problem hiding this comment.
Same as previous with the other user
| } | ||
|
|
||
| // verify that the benefeciary exists | ||
| b, ok := users.Get(beneficiary.String()) |
There was a problem hiding this comment.
It will exist because you created it in line 49, but you're doing the order in reverse. You should first check the user exists, and if it doesn't, create a user for that beneficiary. Same as with the owner.
| // get the user if he exists | ||
| user, ok := users.Get(userAddress.String()) | ||
| if !ok { | ||
| panic("user is not registered") |
There was a problem hiding this comment.
Why panic here but not in line 84? I think it would be better if you add panic in all the possible errors that could happen.
There was a problem hiding this comment.
thanks, didn't pay attention. (modified)
| parts := strings.Split(path, "/") | ||
| switch { | ||
| case len(parts) == 2 && parts[0] == "campaign": | ||
| return renderCampaign(parts[1]) |
There was a problem hiding this comment.
It would be a good practice (and a better customer experience) if you first check if that campaign exists before rendering something that couldn't even be an id for a campaign. (Imagine someone puts campaign/foo).
There was a problem hiding this comment.
do you think what I did is good or not?
There was a problem hiding this comment.
Sorry, my bad. This is correct then.
| var b bytes.Buffer | ||
| c := campaign.(*pkg.Campaign) | ||
| b.WriteString( | ||
| ufmt.Sprintf( |
There was a problem hiding this comment.
Maybe changing this ufmt.Sprintf into 1 line instead of 3? To avoid a long path of 42 lines doing the same and reduce it into 18 (less than the half)
There was a problem hiding this comment.
If you mean the new lines after "ufmt.Sprintf(", I understand. It's because I copied a code as it is from another example. But I'll make the modifications.
There was a problem hiding this comment.
Hey, I left a few comments as a preliminary review.
I think especially the NewUser flow is not necessary, see comments below. Let's use this PR to help you make your code better.
Please decide on design decisions, instead of "to be discussed"; do it how you think it should be, this is your project. If something is to be discussed, it can be done during the review process.
Also please remove unused code and commented out code.
| "time" | ||
| ) | ||
|
|
||
| // ctype: Campaign type, will be an int |
| return c.display | ||
| } | ||
|
|
||
| func (c Contributor) String() string { |
There was a problem hiding this comment.
you could also use the p/demo/json package, up to you.
| func TestNewContributor(t *testing.T) { | ||
| user := NewUser("User", "", testutils.TestAddress("user")) | ||
| amount := int64(500) | ||
| display := true | ||
| date := time.Unix(time.Now().Unix(), 0) | ||
| contributor := NewContributor(user, date, amount, display) | ||
|
|
||
| if contributor.GetUser() != user { | ||
| t.Errorf("Expected contributor user to be %s, got %s", user, contributor.GetUser()) | ||
| } | ||
|
|
||
| if contributor.Date.IsZero() { | ||
| t.Error("Expected contributor date to be non-zero") | ||
| } | ||
|
|
||
| if contributor.Amount != amount { | ||
| t.Errorf("Expected contributor amount to be %d, got %d", amount, contributor.Amount) | ||
| } | ||
|
|
||
| if contributor.GetDisplay() != display { | ||
| t.Errorf("Expected contributor display to be %t, got %t", display, contributor.GetDisplay()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Now that we have uassert & urequire, we should use them
| // Campaign type | ||
| Ctype int | ||
| // Campaign title | ||
| Title string | ||
| // Campaign description | ||
| Description string | ||
| // Campaign goal | ||
| Goal int64 | ||
| // Current amount of money raised | ||
| Current int64 | ||
| // Campaign begin date | ||
| Begin time.Time | ||
| // Campaign deadline | ||
| Deadline time.Time | ||
| // Campaign owner | ||
| Owner *User | ||
| // Campaign beneficiary (can be different from the owner) | ||
| Beneficiary *User | ||
| // Campaign contributors | ||
| contributors []*Contributor | ||
| } |
There was a problem hiding this comment.
I think most of these comments are redundant; ie the name of the variable tells you what it's about. No need to add comments unless there is ambiguity in the name.
| Owner: owner, | ||
| Beneficiary: beneficiary, | ||
| contributors: make([]*Contributor, 0), | ||
| } |
There was a problem hiding this comment.
Generally add newlines between codeblocks
| } | |
| } | |
| if len(c.GetContributors()) != 0 { | ||
| contributors := c.GetContributors() | ||
| for _, contributor := range contributors { | ||
| if contributor.GetDisplay() { | ||
| b.WriteString( | ||
| ufmt.Sprintf( | ||
| "### %s: %dugnot, %s\n", contributor.GetUser().GetName(), contributor.Amount, contributor.Date.String(), | ||
| ), // added the date of contribution in the display, but adds "m=+1726419880.000000001" after the date =>> the problems comes from time.now(), had to modify and use time.Unix(time.Now().Unix(), 0) to solve the problem. | ||
| ) | ||
| } else { | ||
| b.WriteString( | ||
| ufmt.Sprintf( | ||
| "### Anonymous: %dugnot\n", contributor.Amount, | ||
| ), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| return b.String() | ||
| } |
There was a problem hiding this comment.
What if there is a million contributors? I would limit this to the latest 5 - 10, with the option to fetch more somehow.
| banker := std.GetBanker(std.BankerTypeRealmSend) | ||
| mainbal := banker.GetCoins(mainaddr) | ||
| ctype := 0 | ||
| title := "Campaign for Withdrawal" | ||
| description := "Description for Withdrawal" | ||
| goal := int64(1000) | ||
| begin := time.Now() | ||
| deadline := begin.Add(time.Second * 1) | ||
| ownerAddress := testutils.TestAddress("owner") | ||
| beneficiaryAddress := testutils.TestAddress("beneficiary") | ||
| std.TestSetOrigCaller(beneficiaryAddress) | ||
| NewUser("Beneficiary", "") | ||
| std.TestSetOrigCaller(ownerAddress) | ||
| NewUser("Owner", "") | ||
| NewCampaign(ctype, title, description, goal, begin.Unix(), deadline.Unix(), beneficiaryAddress) |
There was a problem hiding this comment.
You can put most of these inside the NewCampaign function. You need to declare them only if you need to use them again (more than once) later
| // Verfiy that the dates are correct | ||
| if beginT < time.Now().Unix() { | ||
| panic("begin date must be in the future") | ||
| } | ||
| if deadlineT < time.Now().Unix() { | ||
| panic("deadline date must be in the future") | ||
| } | ||
| if deadlineT < beginT { | ||
| panic("deadline date must be after begin date") | ||
| } | ||
|
|
There was a problem hiding this comment.
This is redundant and can be simplified; you can check these two things:
- if
beginTis in the future - if
deadlineTis afterbeginT
With transitivity, if:
now < begin && begin < deadline => now < deadline
Also, begin could be >= now
|
|
||
| var owner, beneficiaryU *GFMpkg.User | ||
| // gets the caller address (owner of the campaign) | ||
| caller := std.GetOrigCaller() |
| // banker.IssueCoin(std.CurrentRealm().Addr(), "ugnot", 350000000) | ||
| } | ||
|
|
||
| func NewUser(name string, email string) string { |
There was a problem hiding this comment.
Wouldn't this then be Register or similar?
Not sure you need to have this whole user registration flow; you could just use the addresses as the primary point, where the campaign could have the owner address, name, email, etc, as optional.
Would be more simple that way.
GnoFundMe is a fundraising/crowdfunding platform built with React, TypeScript and Tailwind CSS.
It allows you to connect your Adena Wallet to identify and make actions/transactions.
I created this PR to open a space for discussion/suggestions from anyone that wants to participate and give ideas about this project, the repo of the project with the front is here.
I have this miro where I put my ideas, anyone is welcome to modify and suggest things. Feel free to participate.
Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description