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

feat: implement realm for manual rewards #171

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

harry-hov
Copy link
Contributor

@harry-hov harry-hov commented Sep 26, 2023

Contains initial implementation for manual reward entry
(as mentioned in 2nd point of #159)

Note:
Please check if moving realm -> r/chess breaks anything

@netlify
Copy link

netlify bot commented Sep 26, 2023

👷 Deploy request for gnochess-signup-form pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 0f83b2d

@netlify
Copy link

netlify bot commented Sep 26, 2023

Deploy Preview for gnochess ready!

Name Link
🔨 Latest commit 0f83b2d
🔍 Latest deploy log https://app.netlify.com/sites/gnochess/deploys/65140fa24ef54f0008a8cf2f
😎 Deploy Preview https://deploy-preview-171--gnochess.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Makefile Outdated
@@ -78,13 +78,23 @@ help: ## Display this help message.
cd web; npm run build
cd web; npm run dev

4_deploy_realm: ## Deploy GnoChess realm on local node.
4_deploy_chess_realm: ## Deploy GnoChess realm on local node.
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should just keep a single 4_deploy_realm rule that uploads as many realm as needed (two at the moment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. makes sense. Done.

@moul
Copy link
Contributor

moul commented Sep 26, 2023

if we go with moving realm -> realm/chess; I'd go for r/chess.


// XXX: Update as required
var whitelist = []string{
"g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5",
Copy link
Contributor

@moul moul Sep 26, 2023

Choose a reason for hiding this comment

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

do we already have an admin address shared with the competition operators? if not we should set it now.

cc @thehowl @zivkovicmilos @albttx

"gno.land/p/demo/ufmt"
)

var entries avl.Tree // address -> *RewardEntry
Copy link
Contributor

Choose a reason for hiding this comment

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

It prevents a single address from having multiple entries.

We can survive with this condition, but I think it makes sense for such manual entry realm to support multiple entries per address.

Copy link
Contributor Author

@harry-hov harry-hov Sep 26, 2023

Choose a reason for hiding this comment

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

I don't think it's a good idea to have multiple entries for single address.

Btw you can always override points (increase/decrease).

For some reason we want entry for every points earner. we can have something like:

// RewardEntry represents a reward entry
type RewardEntry struct {
	address std.Address
        totalPoints uint64
	points  []*Point

	updatedAt time.Time
	updatedBy std.Address
}

type Point struct {
        points  uint64
	reason  string
}

Considering the time constrains, I suggest we shouldn't make things unnecessarily complex, unless it is need of the hour

Copy link
Contributor

@moul moul left a comment

Choose a reason for hiding this comment

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

could you add at least one unit test so that gno test runs against the folder and make basic checks like syntax, etc?

I think you can make a simple test adding a few entries and checking the result of Render(); no need to test the whitelisting feature IMO.

@harry-hov
Copy link
Contributor Author

could you add at least one unit test so that gno test runs against the folder and make basic checks like syntax, etc?

Done @moul.

@@ -3,7 +3,7 @@ name: realm
on:
pull_request:
paths:
- "realm/**"
- "r/**"
Copy link
Collaborator

Choose a reason for hiding this comment

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

please rebase and also fix for .github/workflows/deploy-realm.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done -> Can you please verify the changes? -> 0f83b2d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants