Skip to content

Various changes in libs #241

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

Merged
merged 9 commits into from
Aug 10, 2023
Merged

Various changes in libs #241

merged 9 commits into from
Aug 10, 2023

Conversation

MathisGD
Copy link
Contributor

@MathisGD MathisGD commented Aug 8, 2023

  • Harmonize libraries naming with Lib suffix
  • Put WAD definition outside of FixedPointMathLib, because it should be easily accessible from the outside (it will not clash because you are not forced to import it)
  • Harmonize comments in FixedPointMathLib
  • Harmonize fixed point arithmetics functions naming

@pakim249CAL
Copy link
Contributor

pakim249CAL commented Aug 8, 2023

For SafeTransferLib, there are a few options I can suggest that would solve this issue:

  • We can simply replace our IERC20 with solmate's ERC20
  • We can make another lib function that accepts an address or an IERC20, and does the conversion and calls the appropriate SafeTransferLib library functions. Maybe in a special lib named something like "SafeTransferAddressLib"
  • We can just make an internal function to do this.

@Rubilmax
Copy link
Collaborator

Rubilmax commented Aug 8, 2023

What's the issue with using solmate's ERC20?
You can also rename the import

@QGarchery
Copy link
Contributor

What do you think about Errors not being a library ? It would place all constants at top level. Reasons for doing so:

  • shorter names
  • no real risk of name clash (constants in all caps)
  • ErrorsLib feels weird

@Rubilmax
Copy link
Collaborator

Rubilmax commented Aug 9, 2023

What do you think about Errors not being a library ? It would place all constants at top level. Reasons for doing so:

  • shorter names
  • no real risk of name clash (constants in all caps)
  • ErrorsLib feels weird

Features lost:

  • backward solidity compatibility (anyway it's already lost with types being defined at the top level)
  • errors can be all imported using the short import statement instead of having a multi-line import statement for each of the errors used within a file

I'm ok with both solutions

@QGarchery
Copy link
Contributor

Btw, it would be nice to have errors and events handled in the same way. We could even put the errors in IBlue.sol (it works).

For instance, I don't see why events can be imported with a solidity version >= 0.5.0, but errors cannot (currently)

@MathisGD MathisGD self-assigned this Aug 10, 2023
@MathisGD MathisGD changed the base branch from main to feat/amount-and-shares-as-input August 10, 2023 09:38
Base automatically changed from feat/amount-and-shares-as-input to main August 10, 2023 11:39
@MathisGD MathisGD requested review from a team, Rubilmax, MerlinEgalite, pakim249CAL, Jean-Grimal, makcandrov, QGarchery and peyha and removed request for a team August 10, 2023 11:40
@MathisGD MathisGD marked this pull request as ready for review August 10, 2023 11:40
pakim249CAL
pakim249CAL previously approved these changes Aug 10, 2023
MerlinEgalite
MerlinEgalite previously approved these changes Aug 10, 2023
Rubilmax
Rubilmax previously approved these changes Aug 10, 2023
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.

Let's archive morpho-utils at this point

makcandrov
makcandrov previously approved these changes Aug 10, 2023
@MathisGD MathisGD merged commit 8d31147 into main Aug 10, 2023
@MathisGD MathisGD deleted the refactor/libraries branch August 10, 2023 15:12
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.

7 participants