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

Reuse shared logic between Lockup and Flow through a new "SablierBase" interface/abstract #10

Open
PaulRBerg opened this issue Mar 14, 2025 · 2 comments
Labels
effort: high Large or difficult task. 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 Mar 14, 2025

The following functionality is duplicated across the Lockup and the Flow repositories:

  • aggregateAmount
  • collectFees
  • nextStreamId
  • recover
  • setGasTokenAddress
  • updateMetadata

Why not put it into a new shared abstract in this repo, @sablier-labs/evm?

And then, the interfaces for ILockupNFTDescriptor and IFlowNFTDescriptor could be merged into a single interface, again part of this repository.

Note: there's no need to rush this refactor into the current release if there's no time for it.

@PaulRBerg PaulRBerg added effort: high Large or difficult task. 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. labels Mar 14, 2025
@PaulRBerg PaulRBerg changed the title Suggestion to implement a "SablierBase" contract in this repo Reuse shared logic between Lockup and Flow through a new "SablierBase" interface/abstract Mar 14, 2025
@smol-ninja
Copy link
Member

smol-ninja commented Mar 17, 2025

We can move implementations that we are 100% sure won't change over time between Lockup and Flow. But I am not convinced if we should move variables as well.

  • updateMetadata: its inherited through IERC4906. So, the implementation is already outsourced.
  • aggregateBalance and nextStreamId : It would be a bit of a challenge to locate these variables for someone looking at the code for the first time. Therefore, I am not in favour of moving nextStreamId into the shared repo.
  • recover and collectFees: We can. I don't think we would change their implementations between the two protocols. However, it would mean we will have to move aggregateBalance as well.
  • setNativeToken: Sure if we don't change its implementation between the two.

Another argument against moving common code to this repo is that in case we decide to trim down Lockup contract (due to its size constraint) by removing unnecessary functions or refactor some of the functions in Lockup for efficiency reasons, we may revert this change.

Related:

@PaulRBerg
Copy link
Member Author

OK these are good points against merging the code — let's wait for the full spec of the next release to be agreed upon, and then we can revisit this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: high Large or difficult task. 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
None yet
Development

No branches or pull requests

2 participants