Turn registration into NFT #76

niklr · 2021-07-26T13:56:15Z

Issue number

Issue #73

Description

Turn Kickback registration into NFT (aka ticket as NFT)

List of features added/changed

Each event (Conference) has its own NFT contract
Contract supports either ERC721 or 1155
Use Conference.participantsIndex as TokenId
Metadata uri can be set at Conference constructor
Metadata uri is set at Deployer and can be changed by groupAdmin. (Refer to changeClearFee for the similar implementation).
The ticketing NFT cannot be transferred to the address which is already registered (if a user want to buy multiple tickets for friends, the user has to transfer to a different user address before purchasing the second ticket)

How Has This Been Tested?

All of the changes have been tested with unit tests.

When a user registers, ownerOf(tokenId) returns the registrant address.
When the registrant transfers the ticket to a recipient, isRegistered/isAttended/isPaid returns the info about the recipient (if this ended up causing too much attack vector, we may make the token not transferrable ).
When the registrant transfer that ticket to a recipient, the user can register again (as the registrant is no longer registered for the event).
When a user receives the NFT, the user cannot register as the user is already registered.
When the registrant transfers the ticket to a recipient, the new recipient can call withdraw and clearAndSend to receive the payout

Screenshots (if appropriate):

Checklist:

* [x]  My code follows the code style of this project.

* [x]  My code implements all the required features.

contracts/Deployer.sol

contracts/Utils.sol

contracts/ConferenceTicket.sol

makoto · 2021-07-31T17:43:08Z

test/conference_ticket.js

+      let tokenId = 123
+      await ct.tokenURI(tokenId).should.be.rejected
+      await ct.mint(accounts[1], tokenId).should.be.fulfilled
+      await ct.tokenURI(tokenId).should.eventually.eq(tokenId.toString())


Shouldn't this be {baseUri}/{registrantID}?

If the baseTokenURI has not been set it should just return the tokenId.

contracts/test/conference_ticket.js

Lines 101 to 104 in 1bf8b61

let tokenId = 123

await ct.setBaseTokenURI('https://kickback.events/test/')

await ct.mint(accounts[1], tokenId).should.be.fulfilled

await ct.tokenURI(tokenId).should.eventually.eq('https://kickback.events/test/123')

into bounty-nft

niklr · 2021-08-09T14:38:42Z

yarn deploy:polygon

yarn run v1.22.10
$ yarn truffle migrate --network polygon && scripts/local/extractDeployedAddresses.js
$ babel-node node_modules/.bin/truffle migrate --network polygon

Compiling your contracts...
===========================
> Everything is up to date, there is nothing to compile.



Starting migrations...
======================
> Network name:    'polygon'
> Network id:      137
> Block gas limit: 20000000 (0x1312d00)


2_deploy_contracts.js
=====================
No admin addreses set on ./.admins.js
polygon

   Deploying 'EthDeployer'
   -----------------------
   > transaction hash:    0x9367cbcc7017c3976076bdf4060040627d12f18fefe7ac3701b1dc3d5ebc18b5
   > Blocks: 19           Seconds: 40
   > contract address:    0xD874617a94046499367C162D34c5574E8dbBD1e5
   > block number:        17795967
   > block timestamp:     1628519841
   > account:             0x61d64AfBbD3b5CC2D0554A8a92aa5C7540501E7c
   > balance:             2.951254377197681671
   > gas used:            4018426 (0x3d50fa)
   > gas price:           1 gwei
   > value sent:          0 ETH
   > total cost:          0.004018426 ETH


   Deploying 'ERC20Deployer'
   -------------------------
   > transaction hash:    0xa9c84ad5f273a0e90474dd4fa9f3f6676027f34069c6aa14b83e122082c08ee7
   > Blocks: 5            Seconds: 12
   > contract address:    0x9a153fe3790C5fF96BAF7b4A431024920a7E3621
   > block number:        17795972
   > block timestamp:     1628519855
   > account:             0x61d64AfBbD3b5CC2D0554A8a92aa5C7540501E7c
   > balance:             2.947118846197681671
   > gas used:            4135531 (0x3f1a6b)
   > gas price:           1 gwei
   > value sent:          0 ETH
   > total cost:          0.004135531 ETH


   Deploying 'Deployer'
   --------------------
   > transaction hash:    0x9fdd3e7aa159a1b1e8350cb42bd9d6af5656e4e1fbe34b2f8de3dd2aee06806c
   > Blocks: 3            Seconds: 4
   > contract address:    0xA8d0d8Ac811caE275F2bBBBF5FE0e57fEbfe0999
   > block number:        17795977
   > block timestamp:     1628519865
   > account:             0x61d64AfBbD3b5CC2D0554A8a92aa5C7540501E7c
   > balance:             2.946074962197681671
   > gas used:            1043884 (0xfedac)
   > gas price:           1 gwei
   > value sent:          0 ETH
   > total cost:          0.001043884 ETH

,0,0,604800,10
Admin granted to undefined
   > Saving artifacts
   -------------------------------------
   > Total cost:         0.009197841 ETH


Summary
=======
> Total deployments:   3
> Final cost:          0.009197841 ETH


Done in 77.00s.

makoto · 2021-08-11T08:31:06Z

contracts/AbstractConference.sol

     */
    constructor (
        string memory _name,
        uint256 _deposit,
        uint256 _limitOfParticipants,
        uint256 _coolingPeriod,
        address payable _owner,
-        uint256 _clearFee
+        uint256 _clearFee,
+        string memory _baseTokenUri


Rather than passing _baseTokenUri at the constructor, can you save the deployer address and reference the baseTokenUri via Deployer.

For example, define deployer variable

Deployer public deployer;

Then set at the constructor

deployer = msg.sender

then tokenURI becomes like this.

function tokenURI(uint256 tokenId) public view returns (string memory) { return string(abi.encodePacked(deployer.baseTokenURI(), Utils.addr2str(address(this)), '/', _tokenURI(tokenId))); }

Most variables(deposit, cooling period, etc) has to be set at the constructor to make the variable unique to the event.
However, the only time we change baseTokenURI is when we change the metadata server to some other domain so it should be referred to the latest baseTokenURI, rather than passed by the constructor and fixed.

This also has the benefit of reducing changes on frontend/backend code as we are only adding ERC721 related functions, not changing existing interface.

Setting deployer = msg.sender at the constructor will not work because the sender in this case is either EthDeployer or ERC20Deployer and not Deployer. Thus we can't call deployer.baseTokenURI() as suggested.

What about passing the address of Deployer instead of _baseTokenUri at the constructor?

Ah good point. Let's do that

contracts/AbstractConference.sol

makoto · 2021-08-11T09:33:38Z

contracts/AbstractConference.sol

+
+        Participant memory participant = participants[from];
+        participantsIndex[participant.index] = to;
+        participants[from] = Participant(0, address(0), participant.paid);


Should be participants[from] = Participant(0, address(0), false); if you are nullifying everything

makoto · 2021-08-11T09:40:55Z

contracts/AbstractConference.sol

+
+    function safeTransferFrom(address from, address to, uint256 tokenId) public {
+        safeTransferFrom(from, to, tokenId, '');
+    }


Does this need to be overridden? Seems exactly the same as the parent class implementation

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L166

function safeTransferFrom( address from, address to, uint256 tokenId ) public virtual override { safeTransferFrom(from, to, tokenId, ""); }

See the comment https://github.com/wearekickback/contracts/pull/76/files#r686677438

makoto · 2021-08-11T09:51:53Z

contracts/AbstractConference.sol

+        _tokenURIs[tokenId] = Utils.uint2str(tokenId);
+    }
+
+    function transferFrom(address from, address to, uint256 tokenId) public onlyActive {


If you rename transferFrom to _beforeTokenTransfer you don't need to override safeTransferFrom(address from, address to, uint256 tokenId) nor safeTransferFrom(address from, address to, uint256 tokenId, bytes memory _data)

https://docs.openzeppelin.com/contracts/4.x/api/token/erc721#ERC721-_beforeTokenTransfer-address-address-uint256-

v2.5 does not have _beforeTokenTransfer according to https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v2.5.0/contracts/token/ERC721/ERC721.sol

Oh, is it because we still use the old version of the solidity? Ok, then let's leave it as is.

makoto · 2021-08-11T09:53:23Z

contracts/AbstractConference.sol

+
+    function transferFrom(address from, address to, uint256 tokenId) public onlyActive {
+        require(!isRegistered(to), 'already registered');
+        super.transferFrom(from, to, tokenId);


If we use the _beforeTokenTransfer hook, you don't need to call super.transferFrom as it gets called after _beforeTokenTransfer is called.

niklr added 3 commits July 26, 2021 15:44

Turn registration into NFT

c2efb14

Fix module exports

46fba08

Fix lint errors

1bf8b61

makoto reviewed Jul 31, 2021

View reviewed changes

contracts/Deployer.sol Outdated Show resolved Hide resolved

makoto reviewed Jul 31, 2021

View reviewed changes

contracts/Deployer.sol Outdated Show resolved Hide resolved

makoto reviewed Jul 31, 2021

View reviewed changes

contracts/Utils.sol Outdated Show resolved Hide resolved

makoto reviewed Jul 31, 2021

View reviewed changes

contracts/ConferenceTicket.sol Outdated Show resolved Hide resolved

makoto reviewed Jul 31, 2021

View reviewed changes

niklr added 5 commits July 31, 2021 20:50

Remove Utils.toPayable function

6273167

Merge ConferenceTicket into AbstractConference contract

02968e1

Merge branch 'master' into bounty-nft

8cdcc78

Remove ConferenceTicket contract

40674b9

Merge branch 'bounty-nft' of https://github.com/niklr/kickback-contracts

077e57c

into bounty-nft

Unit test nonexistent token

bca17be

niklr requested a review from makoto August 9, 2021 15:24

makoto reviewed Aug 11, 2021

View reviewed changes

contracts/AbstractConference.sol Outdated Show resolved Hide resolved

makoto reviewed Aug 11, 2021

View reviewed changes

contracts/AbstractConference.sol Outdated Show resolved Hide resolved

makoto reviewed Aug 11, 2021

View reviewed changes

Pass deployerAddress instead of baseTokenUri when deploying a conference

857681d

niklr requested a review from makoto August 12, 2021 16:27

makoto approved these changes Aug 14, 2021

View reviewed changes

makoto merged commit 743520b into wearekickback:master Aug 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn registration into NFT #76

Turn registration into NFT #76

niklr commented Jul 26, 2021 •

edited

Loading

makoto Jul 31, 2021

niklr Jul 31, 2021

niklr commented Aug 9, 2021 •

edited

Loading

makoto Aug 11, 2021 •

edited

Loading

niklr Aug 12, 2021 •

edited

Loading

makoto Aug 13, 2021

makoto Aug 11, 2021

makoto Aug 11, 2021

makoto Aug 11, 2021

makoto Aug 11, 2021

makoto Aug 11, 2021

niklr Aug 12, 2021

makoto Aug 13, 2021

makoto Aug 11, 2021

	let tokenId = 123
	await ct.setBaseTokenURI('https://kickback.events/test/')
	await ct.mint(accounts[1], tokenId).should.be.fulfilled
	await ct.tokenURI(tokenId).should.eventually.eq('https://kickback.events/test/123')

Turn registration into NFT #76

Turn registration into NFT #76

Conversation

niklr commented Jul 26, 2021 • edited Loading

Issue number

Description

List of features added/changed

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklr commented Aug 9, 2021 • edited Loading

makoto Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

niklr Aug 12, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklr commented Jul 26, 2021 •

edited

Loading

niklr commented Aug 9, 2021 •

edited

Loading

makoto Aug 11, 2021 •

edited

Loading

niklr Aug 12, 2021 •

edited

Loading