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

Break require into multiple lines. #31

Closed
MicahZoltu opened this issue Dec 27, 2019 · 2 comments
Closed

Break require into multiple lines. #31

MicahZoltu opened this issue Dec 27, 2019 · 2 comments

Comments

@MicahZoltu
Copy link

https://github.com/Uniswap/uniswap-v2-core/blob/d2ddbd95e8500cfd3669ab81b64f48956521ea6b/contracts/UniswapV2.sol#L39

This require statement is pretty gnarly and difficult to grok. Consider breaking it up into multiple require statements like:

require(success, "UniswapV2: TRANSFER_FAILED");
require(data.length == 0 || abi.decode(data, (bool)), "UniswapV2: TRANSFER_FAILED");

A comment here also wouldn't hurt to improve readability, something like:

If the function returned something, assume it was a success flag and check to make sure it is true.

@MicahZoltu
Copy link
Author

Note: This is basically the same as #32, but I wanted to call it out specifically since it is particularly gnarly code and worthy of at least a comment I think, if not the separation.

@NoahZinsmeister
Copy link
Contributor

closing in favor of #32 , let's be consistent with how we handle all of these

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

No branches or pull requests

2 participants