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

Part 3 and 4 Update to UniswapV2Pair() references #7

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

Conversation

5hutt3r5
Copy link

In parts 3 and 4, UniswapV2Pair() does not take arguments, so the references to " pair.**** " were broken. I'm not sure how this worked on the live stream, but to get it to compile for me, I had to change the pair function calls to use abi.encodeWithSignature() using the pair address created by the factory in the initialization. All references to the pair in parts 3 and 4 are updated, so the scripts should now compile out of the box. Since I'm fairly new to Solidity and Echidna, I'd love to get some feedback on this, especially if I missed something in the original implementation.

In parts 3 and 4, UniswapV2Pair() does not take arguments, so the references to "pair.***" were broken. I'm not sure how this worked on the live stream, but to get it to compile, I had to change the pair function calls to use abi.encodeWithSignature() using the pair address created by the factory in the initialization.  All references to the pair in parts 3 and 4 are updated, so the scripts should now compile out of the box. Since I'm fairly new to Solidity and Echidna, I'd love to get some feedback on this, especially if I missed something in the original implementation.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Ziion seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@5hutt3r5 5hutt3r5 changed the title Part 3 and 4 Update EchidnaTest.sol, Setup.sol, and 2 more files... Part 3 and 4 Update to UniswapV2Pair() references Dec 13, 2022
@technovision99
Copy link
Contributor

Hello,
Thanks for contributing! However, I'm not sure what you mean when you say "UniswapV2Pair does not take any arguments." We cast the return address from the factory to be of type UniswapV2Pair when we do UniswapV2Pair(address), rather than deploying a new UniswapV2Pair with new UniswapV2Pair(). We only want to use external testing for the state changing call, as a result for every view function we query we can just use a high-level call. Only the state changing call should come from the User and that's why we have to use the low-level call syntax.

@5hutt3r5
Copy link
Author

5hutt3r5 commented Dec 20, 2022

Update: I'm not sure, but I think my compiler couldn't tell the difference between casting with UniswapV2Pair(address) and calling UniswapV2Pair("constructorArgsHere"). That's what I meant when I said it doesn't take arguments (the constructor expects 0 inputs). I'm not sure how to get it to compile for me without making the changes that I've made because "pair" ends up not being recognized by the compiler in the original version of the code

@technovision99
Copy link
Contributor

Can I see the exact error you are getting and the fixes you have made?

@5hutt3r5
Copy link
Author

Sorry about that, I just tried a fresh repo and it works fine. I'm still not sure what the problem was, but being new to solidity and echidna, I must have inadvertently made a mistake and didn't notice (I may have run echidna with the wrong options or from the wrong folder??). You can check out the changes I made in the pull request if you're interested. Instead of casting the pair address to the type UniswapV2Pair, I used the pair address and it's function signatures to create low level calls to the pair. Correct me if I'm wrong, but I think the functionality should be the same. It was a good learning experience for me either way. Thanks for the help

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.

3 participants