search

Uniswap / v3-periphery

🦄 🦄 🦄 Peripheral smart contracts for interacting with Uniswap v3
https://uniswap.org
GNU General Public License v2.0
1.17k stars 1.09k forks source link

Fix decoding error in Multicall #255

Closed maAPPsDEV closed 1 year ago

maAPPsDEV commented 2 years ago

Summary Of Changes

This PR aims to fix the error in decoding revert reasons that was incorrectly implemented.

Technical Description

Early, a correct solution has been suggested here. But, the solution enforces it encodes the revert reason again when it finally revert at the end. Rather, it doesn't need to repeat decoding and encoding. One of the best efficient solutions might be in OpenZeppelin's Address.sol

Deployment Notes

N/A

Closes #254

stale[bot] commented 2 years ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

maAPPsDEV commented 2 years ago

Hi @fo1981 Thank you for reviewing it. Can I ask if it will be merged soon?

JorgeAtPaladin commented 2 years ago

We completely agree, within our clients we recommend updating the multicall revert to:

assembly {
    revert(add(result,32), mload(result))
}

Just blindly passing the revert bytes seems the most desirable solution in our opinion. The uniswap multicall only works with revert("string") errors and not with revert error(); errors, which in our opinion seems highly undesirable. It also does weird assembly magic as it shifts the error signature into the result.length, which is obviously undesirable.

stale[bot] commented 1 year ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.