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

BrainPass Nft Contract #4

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open

BrainPass Nft Contract #4

wants to merge 55 commits into from

Conversation

OleanjiKingCode
Copy link

Contract Description

  • The contract is called "BrainPassCollectibles" and it represents a non-fungible token (NFT) contract based on the ERC721 standard.
  • The contract allows users to mint and own NFTs called "Brain Passes," which serve as passes for IQ Wiki Editors.
  • Each Brain Pass has a specific pass type with different attributes such as price per day, maximum tokens, and a discount.
  • Users can purchase a Brain Pass by paying the required amount of an ERC20 token (IQ Token) to the contract owner.
  • The contract also supports increasing the time duration of a Brain Pass by paying an additional amount.
  • Users can view their owned NFTs and get details about different pass types.
  • The contract includes functions for adding new pass types, setting the base token URI, and withdrawing funds from the
  • contract.

Linked issues

closes EveripediaNetwork/issues#1358

@Softdev1 Softdev1 removed their request for review May 22, 2023 14:51
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
Comment on lines 55 to 56
uint256 maxTokens;
uint256 discount;
Copy link
Member

Choose a reason for hiding this comment

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

whats maxTokens & discount?

Choose a reason for hiding this comment

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

I think maxTokens should be the max no of NFTS that can be minted (in the case of a limited supply). and for the discount, we felt we might want to give a bit of discount to those subscribing for a long time at once(say a yr subscriptions).

Copy link
Member

Choose a reason for hiding this comment

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

you mention here that the discount param is "to those subscribing for a long time at once". Code doesnt do that at all. it just apply a generic discount to price. which its the same to not having that param and reduce the price per day.

Copy link
Member

Choose a reason for hiding this comment

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

this has never been addressed. Please review my comments before asking for a new code review

Copy link
Member

Choose a reason for hiding this comment

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

third time: please remove discount. it doesnt make any sense current implementation.

if you do a 10% discount its the same that if you just reduce price by 10%.

src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
@kesar
Copy link
Member

kesar commented May 23, 2023

@Emmanueldmlr did u review this contract? There are obvious bugs here like the 1e18 hardcoded and very poor testsuite. Please make sure you carefully review this things before escalates to me.

This looks like a rushed contract with many security flags. A bug in the UI its something that we can easily fix, but a bug in a contract its something it will be irreversible and will damage IQ reputation.

@Emmanueldmlr
Copy link

@Emmanueldmlr did u review this contract? There are obvious bugs here like the 1e18 hardcoded and very poor testsuite. Please make sure you carefully review this things before escalates to me.

This looks like a rushed contract with many security flags. A bug in the UI its something that we can easily fix, but a bug in a contract its something it will be irreversible and will damage IQ reputation.

@kesar , those updates were pushed yesterday night which I haven't reviewed. Initially, we just wanted to get your review about the scope of the contract. I reviewed the contract personally early yesterday which I gave my review to @OleanjiKingCode after the test and I believe he's currently working on resolving them. I will ensure those issues you pointed out are fixed too.

@Emmanueldmlr Emmanueldmlr requested a review from Softdev1 May 23, 2023 09:27
@OleanjiKingCode
Copy link
Author

@kesar most of all the issues here were me correcting and then testing what @Emmanueldmlr saw yesterday and some changes haven't committed, my pc died yesternight

@kesar
Copy link
Member

kesar commented May 23, 2023

@kesar , those updates were pushed yesterday night which I haven't reviewed. Initially, we just wanted to get your review about the scope of the contract. I reviewed the contract personally early yesterday which I gave my review to @OleanjiKingCode after the test and I believe he's currently working on resolving them. I will ensure those issues you pointed out are fixed too.

ok, then @Softdev1 pls ensure you have syncd w emmanuel and both are sure code is good enough to be reviewed. I got today a message from Steve that need my input in this PR and that you already have reviewed it.

If you want the scope of a contract / task I dont need to review the implementation, most likely i would need to review a readme with an interface well documented which its way easier and faster than understanding 400 LoC. 👍🏻

@kesar
Copy link
Member

kesar commented May 23, 2023

@kesar most of all the issues here were me correcting and then testing what @Emmanueldmlr saw yesterday and some changes haven't committed, my pc died yesternight

makes sense. I just see a problem of communication, lets try to get better on that so I dont waste time reviewing a partial contract, and I actually can spend enough time reviewing when you both feel its ready to go.

Again, if you need my input on the functionalities you should have a proper documentation on a readme / interfaces / etc and a proper test suite that helps me to understand how it works 👍🏻

@OleanjiKingCode
Copy link
Author

Okay sir will do a readme doc

@Softdev1
Copy link

No problem sir

@Softdev1
Copy link

@OleanjiKingCode let me know once you are done with the readme doc .

src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
@kesar
Copy link
Member

kesar commented Jun 9, 2023

overall looks good, just some minor improvements 👍🏻

@OleanjiKingCode
Copy link
Author

Okay @kesar I'll make those changes

src/BrainPass/BrainPass.sol Outdated Show resolved Hide resolved
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.

Nft Brainpass Contract
4 participants