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

Mridul Quest 1 Submission #9

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

Conversation

mridul7ahuja
Copy link
Contributor

Quest Code Review

Quest number completed: [1]
Discord handle: [mridz#5250]

Thanks for a great intro task!

A few doubts regarding the tests:
Why was it necessary to change uint to uint256 even though they should be identical?
Why was it necessary to name the address of the Uniswap Exchange V2 Router as 'router' and not anything else?

@ncitron
Copy link
Owner

ncitron commented Jul 27, 2021

This looks great to me!

For your questions:

  1. No real reason for it needing to be named router, we could have named it anything else. The reason the test will fail if you name it anything else though is because router is a public variable. In solidity, all public variables get an automatic external getter function. So in this instance, the getter is called router(). To check if the state variable has been set correctly, we call this getter function, which is why your tests will fail if you do not name it that.
  2. Where did you find using uint instead of uint256 would break the tests? Was it in the function signature during the encoding? This would break it since the use of aliased types are disallowed in calls to abi.encodeWithSignature and uint is just an alias for uint256.

@mridul7ahuja
Copy link
Contributor Author

Yes, you're right I was using the alias in the function signature earlier. Thanks for the detailed explanations!

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.

2 participants