feat(home-realm): add interactive home realm example #2918

stefann-01 · 2024-10-07T05:28:28Z

Description:

This PR finalizes the core functionality of my home realm project, focusing on a dynamic and interactive home realm experience, driven by GNOT donations. The following key features and enhancements are introduced in this PR:

Key Features:

Dynamic Background Change:
- Implemented sequential background changes triggered by GNOT donations. Each donation updates the city background in a fixed order, cycling through a predefined set of cities. The background change is seamless, offering a "traveling" experience for donors.
Sponsor Leaderboard:
- Added a sponsor leaderboard to showcase the top contributors based on their GNOT donations. The list displays the addresses of the top sponsors, formatted for readability (first and last characters with ellipses), and limits the displayed sponsors to the configurable maxSponsors setting.
Donation Validation:
- Introduced a strict GNOT validation check. The system now rejects any donation attempts that don't include GNOT, ensuring only valid contributions update the state of the realm.
Owner Withdrawal of Donations:
- Implemented a feature that allows the realm owner to withdraw accumulated GNOT donations, providing control over the funds contributed by supporters.
Home Realm Configurations:
- Cities: Cities can be dynamically updated to refresh the possible backgrounds.
- Maximum Sponsors: Admins can configure the number of sponsors shown on the leaderboard with the maxSponsors setting.
- Jar Link: Admins can update the link to the donation jar.

codecov · 2024-10-07T13:18:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.97%. Comparing base (e5840e2) to head (f05370c).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2918      +/-   ##
==========================================
+ Coverage   60.94%   60.97%   +0.02%     
==========================================
  Files         564      564              
  Lines       75273    75273              
==========================================
+ Hits        45877    45895      +18     
+ Misses      26021    26009      -12     
+ Partials     3375     3369       -6

Flag	Coverage Δ
contribs/gnodev	`61.46% <ø> (+0.81%)`	⬆️
contribs/gnofaucet	`15.31% <ø> (+0.85%)`	⬆️
gnovm	`65.78% <ø> (ø)`
misc/genstd	`80.54% <ø> (ø)`
misc/logos	`19.88% <ø> (ø)`
tm2	`62.07% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Add profile picture, background image, and about section rendering - Implement dynamic background and tip jar rendering - Add functions to update cities and tip jar link - Integrate sponsor leaderboard - Home realm is still a work in progress

- Ensure city index increments with each donation, resetting after the last city - Implement full GNOT validation to enforce valid donations - Complete sponsor leaderboard functionality with maxSponsors limit - Refine sponsor list display logic to prevent visual issues - Fully integrate tip jar mechanics with donation tracking - Fix AssertAuth issue in config

moul · 2024-10-08T07:54:11Z

examples/gno.land/r/stefann/config/config.gno

+}
+
+func checkAuthorized() error {
+	caller := std.GetOrigCaller()


You should switch to std.PrevRealm.Addr() to prevent someone from creating an intermediary contract that imports yours and calls SetAddress(), which could result in the theft of your ownership.

I explained the solution I chose and the thought process behind it in the comment below. #2918 (comment)

moul · 2024-10-08T07:57:23Z

examples/gno.land/r/stefann/home/home.gno

+	if len(cities) > 0 {
+		currentCityIndex++
+		if currentCityIndex >= len(cities) {
+			currentCityIndex = 0


can be achieved in a single line with %.

btw, should be done when rendering, not when setting; so that if you call UpdateCity with a shorter list, you contract won't break.

For context, what @moul is suggesting:

image := images[index%len(images)]

No need to reset the counter like this manually, the power of math is on your side

zivkovicmilos · 2024-10-08T14:55:57Z

examples/gno.land/r/stefann/config/config.gno

@@ -0,0 +1,53 @@
+package config


Nitpick: this package is called a config, but you do ownership management. Why is it separate?

You should switch to using Ownable, and drop this package entirely

TLDR

The idea behind the config realm was to provide an easy way for others to access the addresses you use. For example, if someone wanted to gift you a realm, they could simply import your config realm and use your pre-defined addresses without needing to ask you for ownership details. This simplifies the process and avoids unnecessary communication. In the end, I decided to use Ownable in my home realm and simplified the config to just handle addresses, getters, and setters, renaming it to Registry. (commit: aeb8340)

Detailed Explanation

I spent way too much time analyzing this, so I feel it’s worth sharing my thought process...

I took the concept of a config realm from @moul and @leohhhn. Their idea (if I got it right) was that the config file could serve as more than just a simple version of Ownable. It would also provide an easy way for yourself and others to access the addresses you regularly use. For instance, if someone wanted to gift you a realm without using Ownable, they could import your config realm and use config.AssertAuthorized() to determine ownership. This removes the need to ask which address you'd like to use for ownership—it’s already specified in the config. This also explains why the config realm isn't part of the home package.

Problems

Leon’s config uses PrevRealm in the AssertAuthorized function, but this doesn’t work as expected. The issue is that the function is meant to be called from the gifted realm. When called, PrevRealm will always return the address of the direct caller. Since, in this case, the PrevRealm call is executed in the config realm rather than the realm being invoked (like with Ownable), it will always return the address of the calling realm (my home realm in this example). As a result, authorization fails, even when the correct owner is calling.

The alternative is to use GetOriginCaller, as @moul does in his config. While this works, it introduces a security risk, as @moul pointed out in his review of my checkAuthorised function. Using PrevRealm for checkAuthorised and GetOriginCaller for AssertAuthorised seemingly fixes the issue for CollectDonations, since this function always sends coins to my address. Even if a malicious intermediary contract is used, no damage can be done. However, this is not the case for all the Update functions used in the home realm. With the attack that @moul described, these functions could be completely exploited by an attacker.

Solutions

One possible solution would be for realms using the config (e.g., my home realm) to manually check ownership each time with something like config.Address() == std.PrevRealm().Addr() or by making AssertAddress take PrevRealm().Addr() as a parameter. However, I didn’t like this approach because it complicates things and goes against the goal of making the use of config straightforward. Additionally, expanding the functionality of the config essentially turns it into a version of Ownable with state and multiple superuser addresses.

This led me to my final solution: rename config to Registry and remove ownership management, leaving it with just two addresses and corresponding getters and setters. This way, someone gifting a realm can use Ownable and set one of the addresses from Registry as the owner.

Ownable Extension Proposal

A useful idea that came out of this process is the potential for an extension of Ownable that supports two superuser addresses, such as a backup address. This would allow a user to recover contracts with their backup address if they lost access to their main address. The Authorizable extension enables multiple addresses but still relies on a single superuser, so it doesn’t fully address this issue.

Feel free to correct me if I got something wrong, and I’d appreciate hearing your thoughts on the Ownable extension idea.

zivkovicmilos · 2024-10-08T14:58:31Z

examples/gno.land/r/stefann/home/home.gno

+func renderTipsJar() string {
+	out := ufmt.Sprintf(`<a href="%s" target="_blank" style="display: block; text-decoration: none;">`, jarLink) + "\n"
+
+	out += `<img src="https://i.ibb.co/4TH9zbw/tips-jar.png" alt="Tips Jar" style="width: 300px; height: auto; display: block; margin: 0 auto;">` + "\n"
+
+	out += `</a>` + "\n"
+
+	return out
+}


Why do you reconstruct this string every time, if it doesn't change?
Isn't it a constant?

I explained the purpose of jarLink and why it can't be a constant in the comment below. #2918 (comment)

Or did you mean to simplify the string concatenation by building the entire string in one call to ufmt.Sprintf? It's done this way for clarity, but I can change it if needed.

zivkovicmilos · 2024-10-08T15:03:14Z

examples/gno.land/r/stefann/home/home.gno

+var (
+	pfp              string
+	cities           []City
+	currentCityIndex int
+	aboutMe          [2]string
+	jarLink          string
+	maxSponsors      int
+	sponsors         []Sponsor
+	totalDonated     std.Coins
+	totalDonations   int
+)


What do you think about grouping these into common objects?

zivkovicmilos · 2024-10-08T15:03:43Z

examples/gno.land/r/stefann/home/home.gno

+		{Name: "London", URL: "https://i.ibb.co/CPGtvgr/london.jpg"},
+	}
+	currentCityIndex = 0
+	jarLink = "https://TODO"


The jarLink is not a leftover; it needs to be updated, using UpdateJarLink function, after the realm is deployed, as it's intended to link to the realm's own Donate function on Studio Connect. I added a comment to clarify its purpose and explain how it should be used (commit: aeb8340).

zivkovicmilos · 2024-10-08T15:05:27Z

examples/gno.land/r/stefann/home/home.gno

+	pfp = url
+}
+
+func UpdateAboutMe(col1, col2 string) {


Why limit the about section to 2 columns?

zivkovicmilos · 2024-10-08T15:14:55Z

examples/gno.land/r/stefann/home/home.gno

+
+	totalDonations++
+
+	sortSponsorsByAmount()


Why should the donator have to pay for sorting the slice?

zivkovicmilos · 2024-10-08T15:24:55Z

examples/gno.land/r/stefann/home/home.gno

+	if len(cities) > 0 {
+		currentCityIndex++
+		if currentCityIndex >= len(cities) {
+			currentCityIndex = 0


For context, what @moul is suggesting:

image := images[index%len(images)]

No need to reset the counter like this manually, the power of math is on your side

zivkovicmilos · 2024-10-08T15:45:56Z

examples/gno.land/r/stefann/home/home.gno

+type SponsorSlice []Sponsor
+
+func (s SponsorSlice) Len() int {
+	return len(s)
+}
+
+func (s SponsorSlice) Less(i, j int) bool {
+	return s[i].Amount.AmountOf("ugnot") > s[j].Amount.AmountOf("ugnot")
+}
+
+func (s SponsorSlice) Swap(i, j int) {
+	s[i], s[j] = s[j], s[i]
+}
+
+func sortSponsorsByAmount() {
+	sort.Sort(SponsorSlice(sponsors))
+}
+
+func GetTopSponsors() []Sponsor {
+	return sponsors
+}


I think you can just keep the top 5 / 10 sponsors in a map, and sort it on each render -- it shouldn't be a problem

zivkovicmilos · 2024-10-08T15:47:55Z

examples/gno.land/r/stefann/home/home.gno

+func CollectDonations() {
+	config.AssertAuthorized()
+
+	banker := std.GetBanker(std.BankerTypeRealmSend)
+
+	ownerAddr := config.Address()
+	banker.SendCoins(std.GetOrigPkgAddr(), ownerAddr, banker.GetCoins(std.GetOrigPkgAddr()))
+}


Why wouldn't Donate automatically forward the funds?

zivkovicmilos · 2024-10-08T15:49:49Z

examples/gno.land/r/stefann/home/home.gno

+func GetTotalDonations() std.Coins {
+	return totalDonated
+}
+
+func GetDonationCount() int {
+	return totalDonations
+}


There is a complete overlap between sponsors, and donations. Why separate these two concepts?

- Remove ownership management logic from `config` and rename it to `registar`. - Integrate `Ownable` for ownership management in the home package. - Add a comment to clarify the purpose of `jarLink` and its expected update process. - Remove the unused `caption` parameter in the `UpdatePFP` function. - Replace `OrigPkgAddr()` with `CurrentRealm()` in `CollectDonations`.

- Run `gno mod tidy` to clean up dependencies. - Run `gno fmt -C ./examples` to format code. - Clean up by fixing the undefined `owner` reference and other minor bugs.

stefann-01 · 2024-10-11T12:54:02Z

I’ve resolved some issues with the latest commits and will be addressing the remaining problems soon. Thank you for your comprehensive suggestions!

stefann-01 requested review from a team as code owners October 7, 2024 05:28

stefann-01 requested review from jaekwon and petar-dambovaliev and removed request for a team October 7, 2024 05:28

github-actions bot assigned stefann-01 Oct 7, 2024

github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 7, 2024

stefann-01 changed the title ~~feat(home-realm): introduce interactive donation-based home realm with background change and sponsor leaderboard~~ feat(home-realm): interactive home realm with background change and sponsor leaderboard Oct 7, 2024

stefann-01 changed the title ~~feat(home-realm): interactive home realm with background change and sponsor leaderboard~~ feat(home-realm): add interactive home realm example Oct 7, 2024

stefann-01 mentioned this pull request Oct 7, 2024

Stefann's Builder Journey gnolang/hackerspace#94

Open

stefann-01 added 3 commits October 7, 2024 19:09

Run make fmt -C ./examples

47c0b81

stefann-01 force-pushed the master branch from 076646a to 47c0b81 Compare October 7, 2024 17:11

moul reviewed Oct 8, 2024

View reviewed changes

Merge branch 'master' into master

f05370c

zivkovicmilos marked this pull request as draft October 8, 2024 14:51

zivkovicmilos reviewed Oct 8, 2024

View reviewed changes

stefann-01 added 3 commits October 10, 2024 22:15

Merge branch 'master' into master

dbda3cf

Run gno mod tidy, fmt, and clean up bugs

b79ba55

- Run `gno mod tidy` to clean up dependencies. - Run `gno fmt -C ./examples` to format code. - Clean up by fixing the undefined `owner` reference and other minor bugs.

stefann-01 added 2 commits October 11, 2024 14:57

Simplify totalDonated addition logic

b0ae489

Rename Registar to Registry

d5c2fe4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(home-realm): add interactive home realm example #2918

feat(home-realm): add interactive home realm example #2918

stefann-01 commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

moul Oct 8, 2024

stefann-01 Oct 11, 2024 •

edited

Loading

moul Oct 8, 2024 •

edited

Loading

zivkovicmilos Oct 8, 2024

zivkovicmilos Oct 8, 2024

stefann-01 Oct 11, 2024 •

edited

Loading

zivkovicmilos Oct 8, 2024

stefann-01 Oct 11, 2024

zivkovicmilos Oct 8, 2024

zivkovicmilos Oct 8, 2024

stefann-01 Oct 11, 2024 •

edited

Loading

zivkovicmilos Oct 8, 2024

zivkovicmilos Oct 8, 2024

zivkovicmilos Oct 8, 2024

zivkovicmilos Oct 8, 2024

zivkovicmilos Oct 8, 2024

zivkovicmilos Oct 8, 2024

stefann-01 commented Oct 11, 2024 •

edited

Loading

feat(home-realm): add interactive home realm example #2918

Are you sure you want to change the base?

feat(home-realm): add interactive home realm example #2918

Conversation

stefann-01 commented Oct 7, 2024 • edited Loading

Description:

Key Features:

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

stefann-01 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

moul Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefann-01 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

TLDR

Detailed Explanation

Problems

Solutions

Ownable Extension Proposal

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefann-01 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefann-01 commented Oct 11, 2024 • edited Loading

stefann-01 commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

stefann-01 Oct 11, 2024 •

edited

Loading

moul Oct 8, 2024 •

edited

Loading

stefann-01 Oct 11, 2024 •

edited

Loading

stefann-01 Oct 11, 2024 •

edited

Loading

stefann-01 commented Oct 11, 2024 •

edited

Loading