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

ToB improvement: emit event when Flashloan taken #731

Merged
merged 4 commits into from
Apr 11, 2023
Merged

ToB improvement: emit event when Flashloan taken #731

merged 4 commits into from
Apr 11, 2023

Conversation

grandizzy
Copy link
Contributor

@grandizzy grandizzy commented Apr 6, 2023 •
edited
Loading

Emit event when Flashloan taken from pool

High level

  • Event is not emitted when flashloan taken from pool
    • Emit Flashloan event when flashloan taken from pool

Description of bug or vulnerability and solution

  • Emit Flashloan event when flashloan taken from pool
  • added Flashloan event containing address of receiver, flashloaned token and amount
  • updated unit tests

Contract size

Pre Change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,442B  (99.45%)
  ERC20Pool                -  23,815B  (96.90%)

Post Change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,519B  (99.76%)
  ERC20Pool                -  23,892B  (97.21%)

Gas usage

Pre Change

N/A

Post Change

N/A

mattcushman
mattcushman approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@mattcushman mattcushman left a comment

Choose a reason for hiding this comment

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

lgtm

MikeHathaway
MikeHathaway approved these changes Apr 6, 2023
View reviewed changes
Copy link
Collaborator

@MikeHathaway MikeHathaway left a comment

Choose a reason for hiding this comment

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

LGTM

EdNoepel
EdNoepel reviewed Apr 8, 2023
View reviewed changes
src/base/FlashloanablePool.sol
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR would be a good place to update the comment on line 24, which should mention amount is a denormalized quantity, dependent upon token precision. This differs from most other pool interactions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated with 1add47c

@grandizzy
Copy link
Contributor Author

Blocked by exceeding contract size

@grandizzy grandizzy requested a review from EdNoepel April 10, 2023 15:55
@grandizzy grandizzy merged commit eed9f63 into develop Apr 11, 2023
@grandizzy grandizzy deleted the flashloan-event branch April 11, 2023 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeHathaway MikeHathaway MikeHathaway approved these changes

@mattcushman mattcushman mattcushman approved these changes

@prateek105 prateek105 Awaiting requested review from prateek105

@ith-harvey ith-harvey Awaiting requested review from ith-harvey

@EdNoepel EdNoepel Awaiting requested review from EdNoepel

Assignees
No one assigned
Labels
Priority: Medium Scope: Edits /src Scope: Style/Consistency
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@grandizzy @MikeHathaway @mattcushman @EdNoepel