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

Specify type(uint256).max behaviors #394

Merged
merged 7 commits into from
Dec 22, 2023
Merged

Conversation

Rubilmax
Copy link
Collaborator

No description provided.

@MerlinEgalite
Copy link
Contributor

conflicts though @Rubilmax

@Rubilmax
Copy link
Collaborator Author

conflicts though @Rubilmax

yes id prefer having the 2 other PRs merged first

src/ERC4626Bundler.sol Outdated Show resolved Hide resolved
@@ -44,7 +44,7 @@ abstract contract ERC4626Bundler is BaseBundler {
/// @dev Initiator must have previously transferred their assets to the bundler.
/// @dev Assumes the given `vault` implements EIP-4626.
/// @param vault The address of the vault.
/// @param assets The amount of assets to deposit. Pass `type(uint256).max` to deposit the bundler's assets.
/// @param assets The amount of assets to deposit. Capped at the bundler's assets.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// @param assets The amount of assets to deposit. Capped at the bundler's assets.
/// @param assets The amount of assets to deposit. Capped at the bundler's balance.

Copy link
Contributor

@QGarchery QGarchery Dec 22, 2023

Choose a reason for hiding this comment

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

I know we use balance for the ERC20.balanceOf elsewhere, but here I think it's useful to make the distinction between assets and shares (since shares are returned by ERC4626.balanceOf below)

Copy link
Contributor

Choose a reason for hiding this comment

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

but it's inconsistent with the other contracts right ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it is inconsistent but still it is clearer in the context of ERC4626

src/TransferBundler.sol Outdated Show resolved Hide resolved
src/migration/AaveV2MigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/AaveV3MigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/AaveV3OptimizerMigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/CompoundV3MigrationBundler.sol Outdated Show resolved Hide resolved
@MathisGD
Copy link
Contributor

I'm in favor of merging this in the other PR and review the other one as the changes are closely related

Base automatically changed from docs/migration to post-cantina December 22, 2023 10:25
@Rubilmax
Copy link
Collaborator Author

I'm in favor of merging this in the other PR and review the other one as the changes are closely related

Sorry i read this after having merged the other but in any case this PR also contains independent changes so IMO it is better to have it independent

@MathisGD MathisGD merged commit 3703741 into post-cantina Dec 22, 2023
8 checks passed
@MathisGD MathisGD deleted the docs/max-uint256 branch December 22, 2023 11:13
@Rubilmax Rubilmax mentioned this pull request Dec 22, 2023
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.

5 participants