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

cleanup,fixes and some improvements #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FarzeenKist
Copy link

Fixes and Improvements

  1. Fixed typos, removed redundant getter functions initialized due to setting mappings to the public (you already created custom getter functions) and code commented out, renamed variables/functions, and added additional comments along with switching function-specific comments into the nat spec format for better documentation
  2. The favorited (now liked) mapping had an issue where it would keep track of the most recent caller of the likeNFT(now likeCName) which could lead to issues where users who had previously liked the CName could like again
  3. Transfer of the NFT representing a CName using safeTransferFrom and transferFrom could lead to issues as the respective struct of the NFT was not being updated when calling these functions. I've gone ahead and fixed that by making these state changes in _beforeTokenTransfer(which is called under the hood by these functions)
  4. Applied the check-effects interaction pattern when buying a CName
  5. Added checks for the name when calling reserveName to prevent users from entering an empty name and also for the imageUri when calling setAddressAvicon
  6. Added the modifier exists to prevent queries of nonexistent CName
  7. Fixed an issue you were having in your hardhat config file
  8. Updated the respective files by renaming the functions of the smart contract

@netlify
Copy link

netlify bot commented Sep 29, 2022

Deploy Preview for celonameservice ready!

Name Link
🔨 Latest commit 9763526
🔍 Latest deploy log https://app.netlify.com/sites/celonameservice/deploys/6335c7be7a393b0009af9540
😎 Deploy Preview https://deploy-preview-1--celonameservice.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 settings.

@eliashezron
Copy link
Owner

I have gone through your PR however, most of your changes, though insightful do not really affect the functionality of the application. especially those on the frontend. Nonetheless, the changes to the UI are vital and much appreciated.

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.

None yet

2 participants