ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.39k stars 5.78k forks source link

immutable for libraries / dynamic libraries / high-level delegatecall #11695

Open chriseth opened 3 years ago

chriseth commented 3 years ago

I'm pretty sure there are issues about this scattered across the project, but this has come up recently, so I'll create a new one.

Since linking is a bit cumbersome, it has been brought up that libraries should be explicit immutable state variables, so that linking happens at construction time instead of before deployment. If we also open this up to non-immutable state variables for libraries, it will create a high-level interface for delegatecall and proxies, even though the library interface cannot be changed after compilation (i.e. for a truly generic proxy, we would need .delegatecall anyway).

Pros:

Cons:

axic commented 3 years ago

it is more complicated for off-chain tools to get the library address (it is not directly in the metadata file, but has to be extracted from the constructor)

Because of this property, I think this feature is rather about having "dynamically linked" (=deploy time) libraries, as opposed to "statically linked" (= compile time).

nventuro commented 3 years ago

I'm not sure the pros list highlights the radical impact that implementing this issue would have. It would let developers fully do away with the notion of linked libraries, which means being able to drop linking tooling, not worry about bogus creation code (pre-replacement), the strange library replacement metadata, more easily expose libraries in use, among others.

With immutable, we can now have a drop-in replacement that still provides all linked library benefits (notably, reducing runtime bytecode size, which is becoming a huge issue in all non-trivial projects), while avoiding all of the weirdness that comes with linking. Libraries can finally be just another dependency passed as a constructor argument, and all contracts would be 'ready' after compilation, making linking a thing of the past.

Seriously, I recently spent a considerable amount of time puzzling over whether it made sense to rely on assembly-level delegatecall rather than use linked libraries due to the sheer number of new edge cases those would force us to cover.


And that's saying nothing about mutable libraries, if we allowed for delegatecalls to be performed on any stack address, whether it came from an immutable variable, a storage one, or even an external call. There's plenty of use cases here that don't involve upgrades, such as allowing external entities to access a contract's storage and do whatever heavyweight processing is required in a view-like manner: this would be a fantastic feature for e.g. price oracles or any other data-storing contract.

But even if this was only for immutable, it would be an incredible win for the language and ecosystem.

it is more complicated for off-chain tools to get the library address (it is not directly in the metadata file, but has to be extracted from the constructor)

I disagree: this instead empowers the developer to provide better ways of querying this information (e.g. a simple public getter), instead of relying on an obscure piece of deta that is not often used for any other purpose.

fvictorio commented 3 years ago

As @nventuro said, the biggest pro here is simplicity. Completely removing this concept of linking seems like a huge win both for tools and for users learning the language.

chriseth commented 3 years ago

I think to avoid confusion, calling non-public (i.e. internal) functions on libraries via a library variable should be disallowed, even if it's immutable. You can still call it via the name of the library, though.

fvictorio commented 3 years ago

@chriseth Does it make sense to keep that behavior of libraries now that free functions exist? More conceptual simplification of libraries would be nice. (I realize this is off-topic for this particular issue; I can create a new one if this idea is not completely off.)

chriseth commented 3 years ago

@chriseth Does it make sense to keep that behavior of libraries now that free functions exist? More conceptual simplification of libraries would be nice. (I realize this is off-topic for this particular issue; I can create a new one if this idea is not completely off.)

We still need #9211 in order to be able to phase out the call behaviour of libraries, but in general I would agree.

hrkrshnn commented 3 years ago

As I understand this, libraries are also deployed during construction time. Is that correct? In that case, for users who rely on libraries to get around contract-size limit would have problems, right?

chriseth commented 3 years ago

The constructor does not automatically deploy any libraries.

hrkrshnn commented 3 years ago

Does this mean that the library address needs to be passed as an argument during the constructor?

viraj124 commented 3 years ago

on first thought as a solidity dev I believe for a contract using multiple libraries would need to have all those libraries deployed first before deploying the contract right might make deployment cumbersome would be good to know more details

chriseth commented 3 years ago

Maybe to clarify: This is just about libraries with public functions, internal functions are a whole different thing. Calling functions on the libraries discussed here will use delegatecall, and not a direct jump.

hrkrshnn commented 3 years ago

During the call, we briefly mentioned the idea of a library interface. The difference being that calls would be delegatecall. I think this might be another approach for this, and can be used as a replacement for some proxy designs that rely on low-level delegate calls.

Syntax could look like:

interface library L {
    function f() external;
    function g() external;
}

Now L can be treated similarly to how regular interfaces can be treated:

  1. they can be stored both as immutable state variable as well as regular state variable (to get upgradability).
  2. new L() isn't well-defined and cannot type check.
  3. L(<address>).f() would be a delegate call instead of call or staticcall.
cameel commented 3 years ago

Do we also want to also document the calling convention for stuff that's not available in contract external functions? E.g. mappings or storage arguments?

We also discussed new L() (in case where it's a library rather than interface). I'm not really sure if that has a good use case. I mean, why would someone deploy multiple copies of a library if it cannot have any state? And if that's just a single copy, then isn't it strictly better to just deploy it rather than deploy a contract that contains its bytecode and can deploy it?

BTW, for anyone who wants to brainstorm this, there's also a topic on the forum: Dynamic linking for Libraries.

nventuro commented 3 years ago

I'm not really sure if that has a good use case. I mean, why would someone deploy multiple copies of a library if it cannot have any state?

The use case that lead to this discussion was me working on a factory contract where the created contracts needed to be linked to a library. Had this existed, I would've run new L() in the factory's constructor.

And if that's just a single copy, then isn't it strictly better to just deploy it rather than deploy a contract that contains its bytecode and can deploy it?

Sure, but that means you now need to send two transactions instead of one, making deployment slightly more difficult. I'm not saying that's a strong enough argument to have it, but if it can be done and it is simple to implement, why not be permissive? Work on this front might also open the door to libraries actually having constructors where they set immutable arguments (e.g. fixed point library with different decimal places), which would IMO be a fantastic development.

cameel commented 3 years ago

The use case that lead to this discussion was me working on a factory contract where the created contracts needed to be linked to a library.

But was that library the same for all created contracts? If so, then wouldn't it be much more efficient (like N+1 vs 2N) to link them all with a single instance of the library deployed beforehand?

Sure, but that means you now need to send two transactions instead of one, making deployment slightly more difficult. I'm not saying that's a strong enough argument to have it, but if it can be done and it is simple to implement, why not be permissive?

The whole language is generally going in the direction of being less permissive in cases that are known to be bad practices :) I think a new feature should at least have some good use cases that redeem it so I'm looking for one. Here it's mostly about cost so maybe you're right and we can afford to be permissive though.

My concern is simply that if you use new, you are basically deploying the library one extra time (as a part of your contract), which is a lot if it's meant to be deployed only once. I think we should not steer people towards doing it by default just because the other way is less convenient. Allowing variables of library types without allowing new would still be enough to make linking easier (you just pass the address to the constructor when deploying).

libraries actually having constructors where they set immutable arguments (e.g. fixed point library with different decimal places), which would IMO be a fantastic development.

Now, that does sound interesting. I did not consider the possibility of immutables in the library itself.

nventuro commented 3 years ago

But was that library the same for all created contracts? If so, then wouldn't it be much more efficient (like N+1 vs 2N) to link them all with a single instance of the library deployed beforehand?

Maybe I didn't explain this situation correctly. Imagine we live in a world where this issue is addressed, libraries can be immutable state variables and linking is not required. What I then have is:

library L { }

contract MultipleCopies {
   constructor(L) { ... } // store immutable reference
}

contract Factory {
  constructor() { l = new L() } // deploy lib to be used for contract creation

  function create() external { new MultipleCopies(l) }
}

With this in place, I don't have to worry about the fact that there's a library involved: the factory deploys it for me.

In particular, deployment of this factory is the same as deployment of a factory that does not use libraries, which is what I was actually going for: I had to extract some functionality out of a contract into a library due to bytecode size issues, but didn't want for that to have a large impact outside of the callsites. Unfortunately, this led me to not only having to deploy another contract as part of the deployment (the library), but also to deal with linking, hence the current conversation.


Again, this was not particularly problematic: I can easily live with a multi-tx deployment. But it seems like such a natural thing to have that disallowing it feels a bit silly.

hrkrshnn commented 3 years ago

About libraries and delegatecalls in general, there are two problems right now.

  1. Solidity currently doesn't provide a high level way to delegatecall a contract. You current need to do address(...).delegatecall(abi.encode(...)) followed by checking if the call succeeded, and the return value (also maybe extcodesize check.
  2. Dynamic linking.

I think problem 1 is much more important than problem 2. If we implement the current proposed solution for problem 2, then I think the solution would be somewhat abused to solve problem 1. Here's an example.

// An interface that we want to delegatecall.
interface I {
    function f(uint) external returns(uint);
}
// A fake library that will be used like an interface.
library L {
    function f(uint) external returns(uint) { }
}
contract C {
    function g(I impl) external {
        uint result = L(address(impl)).g();
    }
}

If we implement this, I think it should be done together with library interfaces.

chriseth commented 3 years ago

One thing to consider is that "using for" does not work for dynamically-linked libraries. The question is whether people are using that with non-internal functions.

github-actions[bot] commented 1 year ago

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

nventuro commented 1 year ago

I stand by my comments here, notably:

I'm not sure the pros list highlights the radical impact that implementing this issue would have. It would let developers fully do away with the notion of linked libraries, which means being able to drop linking tooling, not worry about bogus creation code (pre-replacement), the strange library replacement metadata, more easily expose libraries in use, among others.

With immutable, we can now have a drop-in replacement that still provides all linked library benefits (notably, reducing runtime bytecode size, which is becoming a huge issue in all non-trivial projects), while avoiding all of the weirdness that comes with linking. Libraries can finally be just another dependency passed as a constructor argument, and all contracts would be 'ready' after compilation, making linking a thing of the past.

Seriously, I recently spent a considerable amount of time puzzling over whether it made sense to rely on assembly-level delegatecall rather than use linked libraries due to the sheer number of new edge cases those would force us to cover.

ekpyron commented 1 year ago

Yeah - the way this will go down eventually is probably that we'll fully deprecate and remove the current libraries eventually. As in: we'll replace internal library functions with free functions (grouped into modules for namespacing) and we'll need some other kind of high-level delegatecall mechanism. So the question is rather whether we want to use this issue as umbrella issue for this general plan and protect it from being marked stale or whether we already have another one. We also don't have a concrete plan about when this will happen, but it will happen.