sablier-labs / v2-core

⏳ Core smart contracts of the Sablier V2 token distribution protocol
https://sablier.com
Other
309 stars 45 forks source link

Library events are not included in contract ABIs #215

Closed PaulRBerg closed 1 year ago

PaulRBerg commented 1 year ago

There is currently a bug in Solidity (up to v0.8.17, at least) with errors and events sourced from libraries - they are not included in the final contract ABI, as @razgraf pointed out on Slack recently. See this issue and PR in the Solidity repo:

It looks like this is a WIP, so hopefully Solidity v0.8.18 will fix this. But if it will not, we may have to find an alternative solution, such as:

  1. Move the errors and the events back in the contract interface files
  2. Write a CI script that merges the errors and the events into the contract ABIs

The current approach is to manually paste the errors/ events, but this is not a sustainable solution.

Side note - it looks like Lens Protocol had bumped into the same issue. The error ABIs in the LensHub seem to be hardcoded.

andreivladbrg commented 1 year ago

Altought I consider that the contracts code is better structured with separated libraries for specific things (Errors, Events), if it comes with such a big down side in the subgraph development I'm willing to give up and move them back in the interfaces

PaulRBerg commented 1 year ago

Thanks @andreivladbrg.

On second thoughts, I don't think that moving the errors and the events back to the ISablierV2Linear and ISablierV2Pro interfaces would be super helpful, because the shared errors and events in ISablierV2 would suffer from the same problem still (if I understand the Solidity issue correctly).

PaulRBerg commented 1 year ago

I've been thinking more about his, and I am now of the opinion that we shouldn't do anything about this. We will just have to ask the guys to manually copy-and-paste the error and the event ABIs in the contracts.

Even if we refactor the SablierV2Linear and SablierV2Pro-specific errors back in their respective interface files, the problem would still persist for all custom errors and the remaining events that are not specific (e.g. those defined in ISablierV2). Thus I think that a better use of our engineering efforts would be to work on other more pressing issues and features, such as #246.

FWIW, someone from OpenZeppelin followed up in the Solidity repo issue linked above. I don't think that Solidity will let this issue unresolved for long, so hopefully we will get a fix in Solidity v0.8.18.

Going to mark this as "backlog" for the time being.

PaulRBerg commented 1 year ago

Update: it looks like this bug will be patched in Solidity v0.8.19

https://github.com/ethereum/solidity/issues/13086#issuecomment-1387257613

PaulRBerg commented 1 year ago

It looks like this bug only applies to events, not to custom errors. See my update in the Solidity repo issues tracker:

https://github.com/ethereum/solidity/issues/13086#issuecomment-1417596367

PaulRBerg commented 1 year ago

Based on the latest comments in #354, these are the only two solutions for this bug:

  1. Wait for Solidity to implement a fix (hopefully in v0.8.20).
  2. Define the events within the interfaces.
PaulRBerg commented 1 year ago

It was a good call to fix this bug by moving the events to the interface files (via #359).

The main reason is consistency with the Sablier V2 protocol as a whole; if this bug eventually gets patched by Solidity, then our V2.x deployments will contain events, whereas V2.0 won't.

The other reason is coordination with the wider Ethereum developer ecosystem. Many tools fetch the ABI from Etherscan, e.g. Chisel (see chisel fetch), AnyABI, and even The Graph.

PaulRBerg commented 1 year ago

Yet another benefit of having moved the events to the interfaces; forge doc can auto-generate Markdown documentation for the events:

Screenshot 2023-03-12 at 9 18 33 PM