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.

Sign up for GitHub

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

Jump to bottom

fix/17-complete-natspec #302

Merged
merged 14 commits into from
Oct 30, 2023
Merged

fix/17-complete-natspec #302

merged 14 commits into from
Oct 30, 2023

Conversation

Jean-Grimal
Copy link
Contributor

@Jean-Grimal Jean-Grimal commented Oct 18, 2023 •
edited by MerlinEgalite
Loading

Rubilmax
Rubilmax requested changes Oct 18, 2023
View reviewed changes
src/ethereum/EthereumPermitBundler.sol Outdated Show resolved Hide resolved
src/migration/CompoundV2MigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/CompoundV3MigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/CompoundV2MigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/CompoundV2MigrationBundler.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/AaveV3MigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/AaveV3OptimizerMigrationBundler.sol Outdated Show resolved Hide resolved
@Rubilmax Rubilmax mentioned this pull request Oct 18, 2023
Closed
Rubilmax
Rubilmax reviewed Oct 18, 2023
View reviewed changes
Copy link
Collaborator

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

A few more things and I'm fine

src/StEthBundler.sol Outdated Show resolved Hide resolved
src/StEthBundler.sol Outdated Show resolved Hide resolved
src/TransferBundler.sol Outdated Show resolved Hide resolved
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
@MerlinEgalite MerlinEgalite changed the title fix/17-complte-natspec fix/17-complete-natspec Oct 19, 2023
MerlinEgalite
MerlinEgalite requested changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

Things that seem missing still:

src/ERC4626Bundler.sol Outdated Show resolved Hide resolved
src/ERC4626Bundler.sol Outdated Show resolved Hide resolved
src/MorphoBundler.sol Outdated Show resolved Hide resolved
src/Permit2Bundler.sol Outdated Show resolved Hide resolved
src/migration/CompoundV3MigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/CompoundV3MigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/AaveV2MigrationBundler.sol Show resolved Hide resolved
src/migration/AaveV3MigrationBundler.sol Show resolved Hide resolved
src/migration/AaveV3OptimizerMigrationBundler.sol Outdated Show resolved Hide resolved
src/migration/CompoundV2MigrationBundler.sol Outdated Show resolved Hide resolved
@Jean-Grimal
Copy link
Contributor Author

Things that seem missing still:

Yes but I didn't know what natspec to add for these ones

MerlinEgalite
MerlinEgalite requested changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

I think there was an issue by implementing directly my suggestions 😕

@Jean-Grimal
Copy link
Contributor Author

I think there was an issue by implementing directly my suggestions 😕

Which one ?

@Rubilmax
Copy link
Collaborator

I think there was an issue by implementing directly my suggestions 😕

Which one ?

He means because the CI is failing

@Jean-Grimal
Copy link
Contributor Author

He means because the CI is failing

Weird because they were all about natspec

@MerlinEgalite
Copy link
Contributor

He means because the CI is failing

Weird because they were all about natspec

not I suggested a variable change to conform to blue i remember well

@MerlinEgalite
Copy link
Contributor

Things that seem missing still:

Yes but I didn't know what natspec to add for these ones

You can add this: https://github.com/morpho-org/morpho-blue/blob/7209539d4a0ee968441312a63a5b70dc7b476d38/src/interfaces/IMorpho.sol#L49-L52

@Rubilmax Rubilmax mentioned this pull request Oct 23, 2023
Merged
@MerlinEgalite
Copy link
Contributor

I feel that this PR need to be rebased

@Jean-Grimal
Copy link
Contributor Author

Jean-Grimal commented Oct 24, 2023 •
edited
Loading

I feel that this PR need to be rebased

From review-cantina ?

@Rubilmax Rubilmax reopened this Oct 24, 2023
@Rubilmax Rubilmax marked this pull request as draft October 24, 2023 11:46
@Rubilmax Rubilmax mentioned this pull request Oct 24, 2023
Closed
Jean-Grimal and others added 6 commits October 24, 2023 15:13
@Jean-Grimal @Rubilmax
feat: complete natspec
1968602
@Jean-Grimal @Rubilmax
fix: apply suggestions
0c82911
@Jean-Grimal @Rubilmax
fix: apply suggestions
b626b46
@Jean-Grimal @Rubilmax
fix: apply suggestions
c50d5a6
@Jean-Grimal @Rubilmax
feat: add interfaces natspec
811df2d
@Jean-Grimal @MerlinEgalite @Rubilmax
fix: apply suggestions from code review
29a0d6a 
Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com>
Signed-off-by: Jean-Grimal <83286814+Jean-Grimal@users.noreply.github.com>
StErMi
StErMi reviewed Oct 25, 2023
View reviewed changes
src/ERC4626Bundler.sol Outdated Show resolved Hide resolved
StErMi
StErMi reviewed Oct 25, 2023
View reviewed changes
src/ERC4626Bundler.sol Outdated Show resolved Hide resolved
StErMi
StErMi reviewed Oct 25, 2023
View reviewed changes
src/MorphoBundler.sol Outdated Show resolved Hide resolved
This was referenced Oct 25, 2023
Closed
Merged
@MerlinEgalite
Copy link
Contributor

@StErMi you can review now!

@Rubilmax Rubilmax self-assigned this Oct 26, 2023
@MerlinEgalite
Copy link
Contributor

@Jean-Grimal can you update this branche please?

@MerlinEgalite MerlinEgalite merged commit 4679167 into review-cantina Oct 30, 2023
8 checks passed
@MerlinEgalite MerlinEgalite deleted the fix/issue-17 branch October 30, 2023 16:57
@MerlinEgalite MerlinEgalite linked an issue Nov 3, 2023 that may be closed by this pull request
N-05 Incorrect Documentation #350
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MerlinEgalite MerlinEgalite MerlinEgalite approved these changes

@Rubilmax Rubilmax Rubilmax approved these changes

@StErMi StErMi Awaiting requested review from StErMi

Assignees

@Rubilmax Rubilmax

Labels
☑️ lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

N-05 Incorrect Documentation
4 participants
@Jean-Grimal @Rubilmax @MerlinEgalite @StErMi