flaskr
commented
2 years ago Thanks for the detailed feedback! It's good to wake up to good news. Just to answer a comment that you have:
- ERC721Burnable from Openzeppelin here gives the idea that tokens will be burned lately? That will be good to see how it will work.
The burning of the wrapped token was intended to be part of the mechanism for collection: /* @dev Returns the wrapped token to original lender without further checks. @param tokenId tokenId of the NFT to be sent to original lender / function _returnWrappedTokenToLender(uint256 tokenId) private { delete lendingDurations[tokenId]; address originalLender = wrappedTokenLenders[tokenId]; delete wrappedTokenLenders[tokenId]; _burn(tokenId); emit Collected(tokenId, originalLender); wrappedToken.safeTransferFrom(address(this), originalLender, tokenId); }
But that's an interesting point - on further thought, I don't need that functionality, and it in fact makes it possible for a user to burn a wrapper token, and prevent a collection transaction from being successful - essentially locking the wrapped token in the contract.
Regards, Frank
On Thu, Dec 9, 2021 at 6:28 PM KULDEEP K SRIVASTAVA < @.***> wrote:
Hi Frank – I am Kuldeep, a mentor who is assisting with the grading. Congratulations, your Final Project passed! We'll be approving it on the course itself in a week or two, but here's your feedback in the meanwhile:
Frontend Comments:
- UI is good enough to test the Smart Contract features. Enough details are shown on UI. We can improve a bit on the design.
- Ropsten testnet
- Surge service has been used for front-end deployment. Github pages could have served the purpose too.
- Network change and Account change is unhandled. These can be added.
Testing comments:
- Testcases are structured very properly. Good use of mocha to visually organize the tests.
- Covers all possible scenarios for both of the deployed smart contracts.
- Bonus for Using TypeScript for the testing. It makes it a bit complicated but that is understandable as Hardhat has been used.
Contract Comments:
- Smart Contract work is awesome. Well-detailed comments, design patterns and guard against some possible attacks. Awesome work!
- Inter-Contract Execution: Lender and LenderWrapper contract interact with each other and that is a good usecase of Inter-contract execution. Inheritance-Interfaces: Always good to use interfaces (Openzeppelin or others) to make the contract more secure. The project uses one more design pattern Factory pattern which really works for this idea.
- Reentrancy: Check-effect-interaction is one of the ways to avoid the reentrancy attack but always good to use Reentrancy guard from Openzeppelin. Integer-Overflow-and-Underflow: After solidity version 0.8.0, this is an in-built feature but optional to use so good to use safemath if needed.
- ERC721Burnable from Openzeppelin here gives the idea that tokens will be burned lately? That will be good to see how it will work.
Overall:
- The idea is very new and cool.
- Approval is an important step for this project and providing that on front-end makes it a good user experience.
- Front-end design can be modified to make them look and feel better.
- I liked the idea of lending NFT and using wrapped NFT to make it work. That is still new in the market and maybe later there can be some EIP for it. The project is implemented very nicely and Smart contracts are written very well.
Hope you enjoyed the learning journey so far and can continue building on this.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/flaskr/blockchain-developer-bootcamp-final-project/issues/1, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC34NBGMB2GLX3S5ZM2XE4TUQCAGDANCNFSM5JWC7JOQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
kuldeep23907
Hi Frank – I am Kuldeep, a mentor who is assisting with the grading. Congratulations, your Final Project passed! We'll be approving it on the course itself in a week or two, but here's your feedback in the meanwhile:
Frontend Comments:
Testing comments:
Contract Comments:
Factory patternwhich really works for this idea.Overall:
Hope you enjoyed the learning journey so far and can continue building on this.