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

feat: add accrue fee event #227

Merged
merged 8 commits into from
Oct 20, 2023
Merged

feat: add accrue fee event #227

merged 8 commits into from
Oct 20, 2023

Conversation

Jean-Grimal
Copy link
Contributor

Fix #190

MerlinEgalite
MerlinEgalite requested changes Oct 19, 2023
View reviewed changes
src/libraries/EventsLib.sol Outdated Show resolved Hide resolved
@Jean-Grimal @MerlinEgalite
Update src/libraries/EventsLib.sol
ab9c89d 
Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com>
Signed-off-by: Jean-Grimal <83286814+Jean-Grimal@users.noreply.github.com>
Rubilmax
Rubilmax reviewed Oct 20, 2023
View reviewed changes
Copy link
Contributor

@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.

is this PR expected to be merged in main?

@Jean-Grimal
Copy link
Contributor Author

is this PR expected to be merged in main?

Yes I think

@Rubilmax
Copy link
Contributor

is this PR expected to be merged in main?

Yes I think

alright sorry, forgot it was on MetaMorpho ^^

@MerlinEgalite
Copy link
Contributor

There are some conflicts unfortunately

@MerlinEgalite
Copy link
Contributor

MerlinEgalite commented Oct 20, 2023 •
edited
Loading

It's better to merge main into this branch though @Jean-Grimal

Rubilmax
Rubilmax reviewed Oct 20, 2023
View reviewed changes
test/forge/FeeTest.sol Outdated Show resolved Hide resolved
src/libraries/EventsLib.sol Show resolved Hide resolved
@Jean-Grimal
Copy link
Contributor Author

It's better to merge main into this branch though @Jean-Grimal

That's what I did

@MerlinEgalite
Copy link
Contributor

Oh ok mb this comment misleads me.

Screenshot 2023-10-20 at 13 07 01

Then small tip: run test before finishing the merge to spot any issue.

@Rubilmax Rubilmax merged commit 987537f into main Oct 20, 2023
6 checks passed
@Rubilmax Rubilmax deleted the feat/add-accrue-fee-event branch October 20, 2023 14:55
@Jean-Grimal
Copy link
Contributor Author

Oh no I just finished adding the fee recipient to the event

@Rubilmax
Copy link
Contributor

Oh no I just finished adding the fee recipient to the event

#227 (comment)

I think we shouldn't for now so I'm ok to have this change suggested in a PR not merged for cantina's review unless we settle on https://github.com/cantinasec/review-morpho-blue-1/issues/39 before the start of cantina's review of MEtaMorpho

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

_accrueFee does not log event when fees are accrued, should we?
3 participants
@Jean-Grimal @Rubilmax @MerlinEgalite