Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

update to VRFV2 #681

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

Conversation

pappas999
Copy link

updated Chainlink VRF section to use the new Chainlink VRF V2

@andreipope
Copy link
Collaborator

@pappas999 Thank you for taking the time to create this PR. I see that you updated all the pragma solidity directives to specify version 0.8.7, but the first six lessons still use 0.6.6. Can we update those lessons as well?

@pappas999
Copy link
Author

thanks @andreipope , I updated the previous lessons as well. Everything should be consistent now

Copy link

@TomiOhl TomiOhl left a comment

Choose a reason for hiding this comment

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

Nice work. I have some thoughts on the styling, but nothing major.

en/19/08.md Outdated Show resolved Hide resolved
en/19/08.md Outdated Show resolved Hide resolved
en/19/08.md Outdated Show resolved Hide resolved
en/19/08.md Outdated Show resolved Hide resolved
@pappas999
Copy link
Author

thanks @TomiOhl , I incorporated the feedback given. Please take a look when you can, thanks!

Copy link

@TomiOhl TomiOhl left a comment

Choose a reason for hiding this comment

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

Nice work! Looks fine to me now. Except that I found this one more thing, let's fix up that one too and it'll be ready to go.

en/19/11.md Outdated
function fulfillRandomness(bytes32 requestId, uint256 randomness) internal override {
randomResult = randomness;
function _createZombie(string memory _name, uint _dna) private {
zombies.push(Zombie(_name, _dna));
}

// Delete the function below
Copy link

Choose a reason for hiding this comment

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

To be complete, a "1." should be added here

@pappas999
Copy link
Author

thanks @TomiOhl , i added the change, as well as a couple others I spotted on other pages while doing a check through again.

Copy link

@TomiOhl TomiOhl left a comment

Choose a reason for hiding this comment

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

Nice! Didn't notice them since there wasn't even a blank space after the slashes.
From my side, I think it's ready to merge!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants