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

WIP: Code tweaks #5

Merged
merged 13 commits into from
Aug 1, 2023
Merged

WIP: Code tweaks #5

merged 13 commits into from
Aug 1, 2023

Conversation

ultrasecreth
Copy link
Collaborator

  • Use a struct fro the meta params in the aave wrapper
  • Use a struct fro the meta params in the balancer wrapper
  • Use a struct fro the meta params in the uniswap wrapper

Copy link
Owner

@alcueca alcueca left a comment

Choose a reason for hiding this comment

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

Only minor issues related to natspec and function naming.

src/BaseWrapper.sol Outdated Show resolved Hide resolved
@@ -57,97 +80,44 @@ contract UniswapV3Wrapper is IERC3156PPFlashLender, IUniswapV3FlashCallback {
* @return The amount of `asset` to be charged for the loan, on top of the returned principal.
*/
function flashFee(IERC20 asset, uint256 amount) public view override returns (uint256) {
address pool = address(getPool(asset));
Copy link
Owner

Choose a reason for hiding this comment

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

The natspec above can be copied from a wrapper that has it correct.

src/BaseWrapper.sol Outdated Show resolved Hide resolved

bytes internal _callbackResult;

/// @inheritdoc IERC3156PPFlashLender
Copy link
Owner

Choose a reason for hiding this comment

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

We should @inheritdoc from the ERC7399 interface when possible, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Where does that interface lives? I can't see it in the deps

@ultrasecreth ultrasecreth merged commit b487ae8 into main Aug 1, 2023
2 of 3 checks passed
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