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

Add remix #163

Merged
merged 25 commits into from
Oct 27, 2022
Merged

Conversation

anatoly256
Copy link
Contributor

@anatoly256 anatoly256 commented Sep 20, 2022

Planning to add new resource types (subedition / subedition admin that maintains ledger of nft to subedition) to the core TopShot contract. Specifically, adding a new minting function seems to be the only way to interact with a moments serial number.
https://sketchboard.me/rDsJU4sQGlN#/

WIP
Addresses the following issue:
#164

@joshuahannan
Copy link
Contributor

What is remix? And based off of the conversation we had with Dete about how we want to do composable features in the future, I would potentially think about doing upgrades with new contracts instead of adding a feature with a change to the existing contract

contracts/TopShot.cdc Outdated Show resolved Hide resolved
@jrkhan
Copy link
Contributor

jrkhan commented Sep 22, 2022

A remix is similar in intent to traditional sports trading card parallels.
Specifically, we are interested in minting new Top Shot NFTs that have a subedition with their own serial number range, while still incrementing the total edition count. The subedition will be used to indicate alternate media should be used for the moment.
It's intended that the changes we're making should not break any existing use of the Top Shot contract - no community contracts depending on our contract should need to change as a result.
We will also need a way to map a moment to its sub edition (not yet part of this PR). That mapping will likely need to live in a separate contract. Going to start an issue to continue discussion of the feature.

contracts/TopShot.cdc Outdated Show resolved Hide resolved
@@ -1152,6 +1240,185 @@ pub contract TopShot: NonFungibleToken {
}
}

// getMomentsSubedition function that return's wich Subedition the Moment belongs to
Copy link
Contributor

Choose a reason for hiding this comment

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

getMomentsSubedition returns the Subedition the Moment belongs to

@@ -0,0 +1,21 @@
import TopShot from 0xTOPSHOTADDRESS

// This transaction is for the admin to create a new showcase resource
Copy link
Contributor

@jrkhan jrkhan Oct 14, 2022

Choose a reason for hiding this comment

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

Probably should update comment to indicate it's creating a subedition admin resource

@joshuahannan
Copy link
Contributor

@anatoly256 @jrkhan is this ready for a review again?

@jrkhan
Copy link
Contributor

jrkhan commented Oct 14, 2022

@anatoly256 @jrkhan is this ready for a review again?

Should be ready to review.


// This test tests the pure functionality of the smart contract
func TestSubeditions(t *testing.T) {
b := newBlockchain()
Copy link
Contributor

@jrkhan jrkhan Oct 14, 2022

Choose a reason for hiding this comment

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

Recently added a few helpers that should make this a bit less verbose to setup - take a look at NewTopShotTestBlockchain in topshot_test

contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
@jrkhan jrkhan requested review from tpetrychyn and jrkhan October 17, 2022 21:53
Copy link
Contributor

@jrkhan jrkhan left a comment

Choose a reason for hiding this comment

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

Going to deploy this to testnet to give us a window to start testing contract changes

@@ -94,6 +94,10 @@ pub contract TopShot: NonFungibleToken {
// Emitted when a Moment is destroyed
pub event MomentDestroyed(id: UInt64)

pub event SubeditionCreated(id: UInt32, name: String, metadata: {String:String})
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't clear to me what the ID represents, considering you use subeditionID in the next event. Does it represent the subedition ID? Shouldn't we also have the play and set IDs in the event?

Copy link
Contributor

@jrkhan jrkhan Oct 18, 2022

Choose a reason for hiding this comment

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

Agree with consistent/more specific name for id.
In the current iteration, the subedition is intended to be reused across many sets/plays. (This will make it much easier to search for all 'diamond' moments if they share an id) -> so subedition created event will not have a set/play as the subedition itself does not have a set/play.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub event SubeditionCreated(id: UInt32, name: String, metadata: {String:String})
pub event SubeditionCreated(subeditionID: UInt32, name: String, metadata: {String:String})

contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
contracts/TopShot.cdc Outdated Show resolved Hide resolved
@anatoly256 anatoly256 requested a review from a team as a code owner October 27, 2022 14:12
@anatoly256 anatoly256 changed the base branch from master to feature/add-subeditions October 27, 2022 15:48
@jrkhan jrkhan merged commit 5801b39 into dapperlabs:feature/add-subeditions Oct 27, 2022
jrkhan added a commit that referenced this pull request Oct 27, 2022
* Add remix (#163)

* add remix to contracts

* add remix to transactions

* add remix to lib

* fix misprints

* add function to get moment's subedition

* fix tests

* fix tests

* refactor subEdition contract

* add subEdition tests

* add new function to create subEdition resource

* add documentation

* refactor SubeditionAdmin

* add comments/transactions/tests

* add events

* Revert spacings

* Fix comments

* use test abstractions in subedition_test.go

* fix comments

* fix comments

* add comment to SubeditionAdminStoragePath()

* add subeditionId to MomentMinted event

* change SubeditionCreated event/add length check to MintMoment event

* change subeditionID to subeditionId

* remove spacing changes in assets.go (#178)

Co-authored-by: Jamil Khan <[email protected]>
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.

4 participants