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

fix: remappings with blue anchor (option 3) #520

Closed
wants to merge 5 commits into from
Closed

Conversation

QGarchery
Copy link
Contributor

@QGarchery QGarchery commented Oct 5, 2023

This option is more verbose than option 2 in the imports (who cares though if it is understandable ?), but it has the following benefits:

  • easy to understand, because in this case @blue corresponds to the morpho-blue folder. I find it unexpected to refer to @blue for the dependencies of blue.
  • can be extended to other needs, for example @blue/src can refer to the files defined by the morpho-blue/src folder, same thing with @blue/test

@QGarchery QGarchery requested review from MathisGD and a team October 5, 2023 09:04
@QGarchery QGarchery self-assigned this Oct 5, 2023
@QGarchery QGarchery requested review from Rubilmax, MerlinEgalite and Jean-Grimal and removed request for a team October 5, 2023 09:04
Rubilmax
Rubilmax previously approved these changes Oct 5, 2023
remappings.txt Outdated Show resolved Hide resolved
Co-authored-by: Romain Milon <rmilon@gmail.com>
Signed-off-by: Quentin Garchery <QGarchery@users.noreply.github.com>
MerlinEgalite
MerlinEgalite previously approved these changes Oct 5, 2023
@Rubilmax
Copy link
Collaborator

Rubilmax commented Oct 5, 2023

Note that if morpho-blue is twice a submodule of some repository, only a single version of morpho-blue will be used because of this remapping

So it solves one problem, but not all
The only solution would be to have remapping contexts in forge...

Jean-Grimal
Jean-Grimal previously approved these changes Oct 5, 2023
@Rubilmax
Copy link
Collaborator

Rubilmax commented Oct 5, 2023

Actually this solution has different consequences compared to #517:

In #517, we are creating the convention that morpho-blue dependencies will only use a single remapping in an integrator's repository, essentially pinning morpho-blue's dependencies to a single version in the case where morpho-blue is twice a submodule of the repository

In this PR, we are creating the convention that morpho-blue will only use a single remapping in an integrator's repository, essentially pinning morpho-blue to a single version in the case where morpho-blue is twice a submodule of the repository and that the integrator doesn't override the @blue/src/ remapping (otherwise they are just messing up: they may use 2 distinct versions of blue with the same @blue/lib/ dependencies)

I think this PR is better, because 1 morpho-blue version goes with 1 version of its dependencies, but I just realized that this is a hard convention (unfortunately necessary): all morpho-blue-related remappings will be prefixed @blue/ in any integrator's repository. This is actually similar to npm modules: this way, we choose the package's name in imports

remappings.txt Outdated Show resolved Hide resolved
@Rubilmax
Copy link
Collaborator

Rubilmax commented Oct 5, 2023

In addition, if we want to force the integrator to pin morpho-blue to a single version, then we should use @blue/src/ in contracts instead of relative imports
This way, we force the integrator to use a single version of morpho-blue, even if morpho-blue is twice a submodule of their repository. However, the integrator would only be able to choose the version they want to use by overriding the set of remappings @blue/.../ in their remappings

If we don't do this, the integrator would be able to use 2 versions of morpho-blue but only a single version of its dependencies: the one selected by forge automatically when building remappings = the last remapping @blue/lib/ parsed by forge (which is weird)

@Rubilmax
Copy link
Collaborator

Rubilmax commented Oct 5, 2023

In addition, if we want to force the integrator to pin morpho-blue to a single version, then we should use @blue/src/ in contracts instead of relative imports This way, we force the integrator to use a single version of morpho-blue, even if morpho-blue is twice a submodule of their repository. However, the integrator would only be able to choose the version they want to use by overriding the set of remappings @blue/.../ in their remappings

Wdyt about this @QGarchery?
A practical example is available here: morpho-org/universal-rewards-distributor#62

@QGarchery
Copy link
Contributor Author

Wdyt about this @QGarchery?

I don't know, are you sure that it always forces them to use a single version ? What if the file they use does not import other local files ? What if morpho-blue's versions are compatible in the sense that importing wrong files still compiles ?

It feels a bit like a hack (a nice one !), and I don't think it's our responsibility to ensure that integrator don't have multiple versions of morpho-blue. Plus I like to have the flexibility to use relative imports (they read and interact nicely)

MathisGD
MathisGD previously approved these changes Oct 5, 2023
@Rubilmax
Copy link
Collaborator

Rubilmax commented Oct 5, 2023

I don't know, are you sure that it always forces them to use a single version ? What if the file they use does not import other local files ? What if morpho-blue's versions are compatible in the sense that importing wrong files still compiles ?

It feels a bit like a hack (a nice one !), and I don't think it's our responsibility to ensure that integrator don't have multiple versions of morpho-blue. Plus I like to have the flexibility to use relative imports (they read and interact nicely)

That is correct: they can always use relative imports
But if they compile some contract of morpho-blue that imports another using @blue/lib/, then this remapping will be the last one parsed by forge among all morpho-blue versions (and the integrator can't do anything because it is morpho-blue code)

The solution of the @blue/src/ remapping doesn't force the integrator to use a single version of morpho-blue indeed, but it makes the work more difficult to mess up with morpho-blue versions because they are expected to import morpho-blue using @blue/src/ remappings, which would only point to a single version

test/forge/libraries/UtilsLibTest.sol Outdated Show resolved Hide resolved
remappings.txt Outdated Show resolved Hide resolved
@QGarchery
Copy link
Contributor Author

It still has the issue that if a project imports morpho-blue twice, then the same version of the dependencies will be used for both

@QGarchery QGarchery closed this Oct 6, 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.

6 participants