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

Move the precompiles to a new directory that is shipped to npm #789

Closed
2 of 3 tasks
PaulRBerg opened this issue Jan 10, 2024 · 11 comments
Closed
2 of 3 tasks

Move the precompiles to a new directory that is shipped to npm #789

PaulRBerg opened this issue Jan 10, 2024 · 11 comments
Assignees
Labels
effort: medium Default level of effort. priority: 2 We will do our best to deal with this. type: refactor Change that neither fixes a bug nor adds a feature. work: clear Sense-categorize-respond. The relationship between cause and effect is clear.

Comments

@PaulRBerg
Copy link
Member

PaulRBerg commented Jan 10, 2024

As discussed here, it would be helpful to ship the precompiles so that we can write a protocol deployment script that spans V2 Core and V2 Periphery.

Task:

  • Move the precompiles in the precompiles directory
  • Include the precompiles in the npm package
  • Include the script/Precompiles.s.sol file in the npm package (via files)

Food for thought: or maybe we shouldn't include the precompiles in script? How about another directory, e.g., precompiles?

I will let you @andreivladbrg and @smol-ninja self-organize and assign this.

@PaulRBerg PaulRBerg added type: refactor Change that neither fixes a bug nor adds a feature. priority: 2 We will do our best to deal with this. work: clear Sense-categorize-respond. The relationship between cause and effect is clear. effort: medium Default level of effort. labels Jan 10, 2024
@andreivladbrg
Copy link
Member

andreivladbrg commented Jan 10, 2024

or maybe we shouldn't include the precompiles in script? How about another directory, e.g., precompiles?

interesting idea, i like it, i've got just one concern, would foundry compile the contracts from a not known path? or does foundry compile contract only from the configured paths? not sure if it is possible to override test:

[profile.default]
  src = "src"
  test = "test"
  test ="precompiles"

@andreivladbrg andreivladbrg self-assigned this Jan 10, 2024
@smol-ninja
Copy link
Member

Looks like by setting allow_paths, we can instruct foundry to read solidity files from additional directories.

@andreivladbrg
Copy link
Member

oh, ok, thanks

thought that allow_paths is only for remappings

@PaulRBerg
Copy link
Member Author

AFAIK, Foundry will be able to read the contracts even without allow_paths defined. Anyway - something that should be easy to test, @andreivladbrg.

@PaulRBerg PaulRBerg changed the title Move the precompiles to script Move the precompiles to a new directory that is shipped to npm Jan 11, 2024
@smol-ninja
Copy link
Member

If we haven't yet decided on the directory for precompiles, how about using src/precompiles for that?

My Rationale behind it

  1. Having it in script appears to be similar to having it in test/utils as both of them are not directly related to the source code. While script stores deployment scripts, test/utils stores utility functions for testing.
  2. All the imports in precompile are from src so it looks reasonable to put it in the src directly.

@PaulRBerg
Copy link
Member Author

That's an interesting idea, @smol-ninja.

One wrinkle with putting it in src is that it would break the topology of the current subdirectories there.

That is:

  • abstracts
  • interfaces
  • libraries
  • types

precompiles would not be at the same "logical level" with any of the foregoing categories. The precompiles encompass everything in src.

Thus, I would be slightly more inclined to put the precompiles in a new folder called precompiles that sits at the same level as src.

@smol-ninja
Copy link
Member

Thanks so much for explaining it @PaulRBerg. After reading your comment, I am now in favour of moving it to precompiles folder.

@smol-ninja smol-ninja moved this to 📋 Not Started in Lockup 1.2.0 Mar 4, 2024
@smol-ninja smol-ninja moved this from 📋 Not Started to 🏗 In progress in Lockup 1.2.0 Mar 6, 2024
@smol-ninja
Copy link
Member

smol-ninja commented Mar 8, 2024

Clarifications:

  1. We need script/Precompiles.s.sol script in v2-periphery and not in v2-core, right?
  2. To include Precompiles.s.sol script in the npm package, It should inherit from Script and not BaseScript otherwise we would also have to include Base.s.sol in the npm package, right? Or do we want to include Base.s.sol?

AFAIK, Foundry will be able to read the contracts even without allow_paths defined.

That's correct.

@smol-ninja smol-ninja moved this from 🏗 In progress to 👀 In review in Lockup 1.2.0 Mar 9, 2024
@smol-ninja smol-ninja moved this from 👀 In review to ✅ Done in Lockup 1.2.0 Mar 11, 2024
@PaulRBerg
Copy link
Member Author

PaulRBerg commented Mar 28, 2024

Sorry @smol-ninja, I had missed your questions here.

I confess that I don't understand your questions.

  1. The fact that I've been away from the code base for many months is telling. Why do we need script/Precompiles.s.sol?
  2. Precompiles.sol does NOT inherit from either Script or BaseScript.

Could you please re-explain? Your questions might have referred to an older state of the code base.


More context: I am looking at the staging branches in V2 Core and V2 Periphery and the way the precompiles have been refactored looks great to me.

@smol-ninja
Copy link
Member

No problem @PaulRBerg. I am happy to explain it again.

The fact that I've been away from the code base for many months is telling. Why do we need script/Precompiles.s.sol?

Well it was in the OP (I struckthrough it). Iirc, the idea was to ship it with npm package so that developers can use it to deploy the entire protocol using precompiles.

Precompiles.sol does NOT inherit from either Script or BaseScript.

Yes thats correct. The question was about Precompiles.s.sol.

@PaulRBerg
Copy link
Member Author

Ooh, I see.

I think what I forgot is that Precompiles.s.sol is a hypothetical script that we have never written. Then, you made this remark that settled the matter insofar we realized that a protocol-level deployment script isn't feasible from a gas POV.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Default level of effort. priority: 2 We will do our best to deal with this. type: refactor Change that neither fixes a bug nor adds a feature. work: clear Sense-categorize-respond. The relationship between cause and effect is clear.
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants