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

Fast Break #229

Merged
merged 43 commits into from
Jan 9, 2024
Merged

Fast Break #229

merged 43 commits into from
Jan 9, 2024

Conversation

HouseOfHufflepuff
Copy link
Contributor

@HouseOfHufflepuff HouseOfHufflepuff commented Dec 10, 2023

Fast Break is a game built on the NBA Top Shot contract. Players of Fast Break submit moments in their Top Shot collection to play the game. Each Fast Break contains n FastBreakStatistic the wallet must match or exceed.

When a player submits a lineup to Fast Break, a game token is minted to their Fast Break collection. This game token contains their submission data and links to the game. As NBA games are played, the metadata of the token (points & win) are updated.

A Fast Break Run is a block of Fast Breaks. Winning a Fast Break puts you on the Fast Break Run leaderboard.

The resource FastBreakDaemon is reserved for the game oracle. The oracle is responsible for bringing trusted NBA data into its platform, computing points & wins, and updating the blockchain with the derived data. The FastBreakDaemon keeps the on-chain & off-chain games in sync.

Custodial submissions to Fast Break will be made by the game oracle in batch. Non-custodial submissions can be made by any wallet with Top Shots.

Screen Shot 2023-12-17 at 7 05 20 PM

@HouseOfHufflepuff HouseOfHufflepuff marked this pull request as ready for review December 18, 2023 03:05
@HouseOfHufflepuff HouseOfHufflepuff requested a review from a team as a code owner December 18, 2023 03:05
@HouseOfHufflepuff HouseOfHufflepuff changed the title [DRAFT] Fast Break On-Chain Fast Break On-Chain Dec 18, 2023
Copy link
Contributor

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Can you add comments to all your types, fields, and functions in the contract? That would make the review process much easier for us. Thank you!

contracts/FastBreak.cdc Show resolved Hide resolved
@HouseOfHufflepuff HouseOfHufflepuff changed the title Fast Break On-Chain Fast Break Dec 19, 2023
@HouseOfHufflepuff
Copy link
Contributor Author

HouseOfHufflepuff commented Dec 29, 2023

Please correct me if I am wrong, but it looks like the actual NFTs that you are defining here are just meant to only be used in a single game and are not used any time afterwards? If that is the case, it doesn't really make sense to make them the NFTs and it would make a lot more sense to make the NFT represent the player instead of the game, so it can be used indefinitely across multiple different games, can be moved to a new address if the player wants to for whatever reason, can be traded or sold on marketplaces, and can contain meaningful metadata about the player' history.

In addition, you'll also need to have this contract implement ViewResolver and use metadata views for the NFTs and the contract

Yes, the intention is to create a game token that represents a single play of Fast Break. I chose this design because it maximizes the flexibility other contracts can have building on-top of Fast Break.

Having said that, I really like the idea of having a FastBreakPlayer resource or NFT. I think it adds a ton to the game. I started off creating it as a resource over here, but I am thinking it might be better as an NFT in its own contract. Would appreciate advice or collaboration on this PR on the FastBreakPlayer concept.

I plan to implement ViewResolver when we have a visual design for this game token. Until then, would like to hide it. Am working with Julian on design but needs to be prio'd.

#237

Copy link
Contributor

@tpetrychyn tpetrychyn left a comment

Choose a reason for hiding this comment

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

Reviewed via a slack huddle, LGTM as a v1

contracts/FastBreak.cdc Outdated Show resolved Hide resolved
Copy link
Contributor

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Just left a few reminders on some of bastians original review comments. Lets get those fixed now, and I'll be able to get to a full review soon

contracts/FastBreak.cdc Outdated Show resolved Hide resolved
contracts/FastBreak.cdc Outdated Show resolved Hide resolved
contracts/FastBreak.cdc Outdated Show resolved Hide resolved
Copy link
Contributor

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

So I take it that you don't want to made the player a resource instead of tracking things by account address? It doesn't seem like you made any significant changes since the last time I looked at this

@HouseOfHufflepuff
Copy link
Contributor Author

@joshuahannan, I want to make the Player an NFT in a new contract and started working on it in the PR mentioned in my comment. Having said that, I do think a game token adds more to the goals of the game, is more experimental, and more composable, and I am planning to stick with this concept. However, I do like the idea of a player NFT as well, but correct me if I am wrong, that would need to be in a different contract.

Either way, the core functionality I want in this contract is present in this PR. Have made all the suggestions Bastian made afaik. Thanks again for your time.

#229 (comment)

@joshuahannan
Copy link
Contributor

oh sorry, I think I missed that last part of your comment last week. I disagree that you are able to implement the player as a resource in a different contract. You're already tracking players by address in this contract, so if you were to try to graft a resource from a separate contract onto this as a player, it wouldn't work IMO because it can never be reconciled with the address tracking that you're doing here. It is a fundamental enough part of the game that you would need to track players by only their resource IDs and remove address tracking completely. That is also a more Cadence-best practices way to do it.

@HouseOfHufflepuff
Copy link
Contributor Author

Hey @joshuahannan, sorry for the delay as I am working on this over weekends. I implemented the Player resource and removed wallet data in favor of a resourceId. I like the concept a lot and feel like this really improved the contract. Thanks for pushing me to do this. Would appreciate your re-review.

Copy link
Contributor

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Looks good. I like the player resource model better. One thing you do need to be aware of with this model is that because most web3 people still care so much about addresses, people are probably still going to want to be able to see stats based on addresses instead of player IDs because they're stuck in the old ways and don't understand that tying things to resources is much better in terms of composability and extensibility. unfortunately, the'll want it either way.

There are a few ways you could handle this. You could just say no and tell people to adjust to this new model. That might work, but you would need to push this model pretty heavily in the design of the user facing stats and game. You could also emit addresses in events and such like we do with Withdraw and Deposit to at least have the events include an address. You just can get that from a player or collection resource by doing self.owner?.address.

Or, you could make all this information more accessible on-chain by having a mapping of playerIDs to addresses to show which address owns which player at any given time. You could keep this updated by just putting a line in one of the player methods that just keeps it updated automatically.

contracts/FastBreak.cdc Outdated Show resolved Hide resolved
contracts/FastBreak.cdc Show resolved Hide resolved
HouseOfHufflepuff and others added 3 commits January 9, 2024 09:59
@HouseOfHufflepuff HouseOfHufflepuff merged commit 063eda5 into master Jan 9, 2024
3 checks passed
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.

6 participants