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

Package tethering #808

Open
3 of 5 tasks
PaulRBerg opened this issue Jan 30, 2024 · 29 comments
Open
3 of 5 tasks

Package tethering #808

PaulRBerg opened this issue Jan 30, 2024 · 29 comments
Assignees
Labels
effort: epic Multi-stage task that may require multiple PRs. priority: 2 We will do our best to deal with this. type: refactor Change that neither fixes a bug nor adds a feature. work: complex Probe-sense-respond. The relationship between cause and effect can only be perceived in retrospect.

Comments

@PaulRBerg
Copy link
Member

PaulRBerg commented Jan 30, 2024

As discussed in https://github.com/sablier-labs/company-discussions/discussions/25

  • Move all the V2 Periphery contracts to an Airdrops rpeository
  • Update all stale references (e.g. turn "V2 Core" into "V2 Lockup")
  • Remove the "V2" from the contract names (note: for ASCII, you can use the "Dark with Shadow" font on this tool)
  • Don't forget about the related stuff, e.g. the Wikis
  • Rename this repo to lockup
@PaulRBerg PaulRBerg added effort: epic Multi-stage task that may require multiple PRs. priority: 0 Do this first before everything else. This is critical and nothing works without this. work: complex Probe-sense-respond. The relationship between cause and effect can only be perceived in retrospect. type: refactor Change that neither fixes a bug nor adds a feature. labels Jan 30, 2024
@andreivladbrg
Copy link
Member

In advance, I want to mention that this change will likely take a longer amount of time.

@PaulRBerg
Copy link
Member Author

Yep. That's why I've marked it as effort: epic.

@andreivladbrg
Copy link
Member

andreivladbrg commented Jan 30, 2024

just noticed this:

Rename this repo to v2-lockup

wasn't the idea to also remove "v2" from the repo name? i would suggest lockup-contracts, maybe?

from here:

AVB:
image

PRB:

image

@PaulRBerg
Copy link
Member Author

Oh shoot, you're right @andreivladbrg.

The repo should be renamed to evm-lockup or sablier-lockup.

I would go with the latter, though I'm on the fence about it. Which one do you like more? Cc @smol-ninja.

@smol-ninja
Copy link
Member

I am more inclined towards calling it sablier-lockup. My rationale is that Sablier may not necessarily be compatible with different types of zkEVMs, such as type 3 and type 4 1 without modifying some parts of it. Even though they are not in practice yet and not very popular, calling it evm-lockup wouldn't do justice to the name.


Since this is going to be an epic level of effort and likely to take longer time (as Andrei correctly pointed out), there could be a huge number of conflicts between changes introduced by this and other PRs raised during the same period, should we split it into multiple issues for a smooth transition from current architecture to PacTet architecture?

Footnotes

  1. zkEVMs are also EVMs.

@PaulRBerg
Copy link
Member Author

That's a good point, @smol-ninja, but after more rumination on this, I think we should go with evm-lockup.

  1. Even if type-3/4 zkEVMs are not full-blown EVMs, they are still fundamentally EVMs. evm-lockup would be a good first approximation.
  2. If we use sablier-lockup, how would we name a non-EVM Lockup implementation? solana-lockup? Naming it like that would break the consistency in naming, i.e., one repo would start with the project name ("Sablier"), the other with the blockchain name ("Solana").

WDYT?

should we split it into multiple issues for a smooth transition from current architecture to PacTet architecture?

I am open to that, but I would still prefer to keep this "parent" issue to track all the other "child" issues. We could replace the task list in the OP with references to the other isuses.

@smol-ninja
Copy link
Member

smol-ninja commented Jan 31, 2024

Fair point about evm-lockup. I agree with both of your arguments.

I am open to that, but I would still prefer to keep this "parent" issue to track all the other "child" issues. We could replace the task list in the OP with references to the other issues.

Yes, thats what I had in my mind. We can split into the following sub-issues (suggestions welcome):

  • Move all the V2 Periphery contracts here and integrate with the core repo
  • Remove "V2" from the contracts
  • Update ERC721 name to Sablier Lockup Linear v1.0.1 NFT (in case of Lockup Linear)
  • Rename this repo to evm-lockup and refactor the contract names
    • Update all references in the code such as function names, variables etc.
    • Update markdown guidelines
  • Update Wikis
  • Update Sablier docs

@andreivladbrg do you have any comment on this?

@PaulRBerg
Copy link
Member Author

@smol-ninja sounds good.

@andreivladbrg
Copy link
Member

Move all the V2 Periphery contracts here and integrate with the core repo
Rename this repo to evm-lockup and refactor the contract names
Update all references in the code such as function names, variables etc.
Update markdown guidelines

between these two, we should remove "V2" from the contracts

one other thing bad with the sablier-lockup name is that it would be redundant to have "sablier" twice in the github URL: sablier-labs/sablier-lockup

@PaulRBerg
Copy link
Member Author

we should remove "V2" from the contracts

yeah, that should be another issue

one other thing bad with the sablier-lockup name is that it would be redundant to have "sablier" twice in the github URL: sablier-labs/sablier-lockup

That's not bad, actually. The name "sablier" would appear on more computers.

@smol-ninja
Copy link
Member

As discussed on Slack, this will be picked after major refactoring and LockupTranched contracts are finished.

@PaulRBerg
Copy link
Member Author

Another implication of PacTet is that we will have to update the name of our NFT collections on Etherscan.

For instance, this collection should be renamed from "Sablier V2 Lockup Linear NFT" to "Sablier Lockup Linear v1.0.1 NFT".

@smol-ninja
Copy link
Member

The name is not related to Etherscan. It's ERC721 metadata set up in our core contracts.

@PaulRBerg
Copy link
Member Author

The name is not related to Etherscan

Actually, it's also related to Etherscan. The NFT collection has to be manually listed (which is what I did a while ago).

but yes you're right that we also have to update the metadata in the Solidity code.

@smol-ninja
Copy link
Member

Original issue #820.

The task is to adjust the description generated in the NFT descriptor to account for the package tethering, i.e., say LockupLinear v1.1.2 instead of Sablier V2:

return string.concat(
"This NFT represents a payment stream in a Sablier V2 ",
streamingModel,
" contract. The owner of this NFT can withdraw the streamed assets, which are denominated in ",
assetSymbol,
".\\n\\n- Stream ID: ",
streamId,
"\\n- ",
streamingModel,
" Address: ",
sablierAddress,
"\\n- ",
assetSymbol,
" Address: ",
assetAddress,
"\\n\\n",
unicode"⚠️ WARNING: Transferring the NFT makes the new owner the recipient of the stream. The funds are not automatically withdrawn for the previous recipient."
);
}

@PaulRBerg PaulRBerg pinned this issue Mar 28, 2024
@smol-ninja
Copy link
Member

Since open-ended and core are going to share the same periphery contracts, shouldn't we include open-ended repo into this as well? Ofc, we can choose to not include it but what are your thoughts on this?

@PaulRBerg
Copy link
Member Author

Do you mean include open-ended in the repo currently called v2-core?

@smol-ninja
Copy link
Member

Yes thats what I meant, or we build separate periphery contracts for the open-ended, i.e. v2-lockup contains core and periphery required for lockup product and open-ended repo contains core and periphery required for open-ended.

@PaulRBerg
Copy link
Member Author

Got it.

I need to review open-ended before I am able to properly comment on this. But my instinct is to keep them separated.

@andreivladbrg
Copy link
Member

andreivladbrg commented Apr 18, 2024

Since open-ended and core are going to share the same periphery contracts

this is not correct, this is why we have renamed the batch contract to BatchLockup

shouldn't we include open-ended repo into this as well? Ofc, we can choose to not include it but what are your thoughts on this?

i am not in favor of this, since the repo name would contain lockup and the package versions are not synced

@smol-ninja
Copy link
Member

i am not in favor of this

Me neither. I also think we should keep OE separate from lockup repo.

@andreivladbrg
Copy link
Member

great, thanks for confirming

@smol-ninja smol-ninja added priority: 2 We will do our best to deal with this. and removed priority: 0 Do this first before everything else. This is critical and nothing works without this. labels Jun 14, 2024
@andreivladbrg andreivladbrg self-assigned this Jul 19, 2024
@smol-ninja
Copy link
Member

@PaulRBerg should we rename SablierNFTDescriptor to LockupNFTDescriptor or SablierLockupNFTDescriptor given that NFT Descriptor for Flow would be separate?

@PaulRBerg
Copy link
Member Author

The jury is still out if it will be separate. But let's go with LockupNFTDescriptor for now.

@smol-ninja
Copy link
Member

smol-ninja commented Aug 1, 2024

The refactor PR has been merged so now we can move ahead with changing the repo name.

@andreivladbrg
Copy link
Member

The refactor PR has been merged so now we can move ahead with changing the repo name.

wait, i think we shouldn't rename it until staging is merged to main - i.e. when we release the next version

@smol-ninja
Copy link
Member

I understand your point—it would be odd to have two repos tied to the current release: lockup and v2-periphery.

While writing this, I thought, what if we archive both v2-core and v2-periphery as they are, and then fork v2-core (including all branches) into a new lockup repo? What do you think?

@andreivladbrg
Copy link
Member

andreivladbrg commented Aug 2, 2024

I understand your point—it would be odd to have two repos tied to the current release: lockup and v2-periphery.

While writing this, I thought, what if we archive both v2-core and v2-periphery as they are, and then fork v2-core (including all branches) into a new lockup repo? What do you think?

hmm, interesting idea, not sure yet what to say

my point is to not rush with the decision

@PaulRBerg
Copy link
Member Author

I suggest renaming this repo to lockup instead of creating a separate one. The rationale is thus:

  • We would lose the 310+ stars
  • We would lose the connectivity enabled by the historical backlinks (on X, Telegram, etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: epic Multi-stage task that may require multiple PRs. priority: 2 We will do our best to deal with this. type: refactor Change that neither fixes a bug nor adds a feature. work: complex Probe-sense-respond. The relationship between cause and effect can only be perceived in retrospect.
Projects
None yet
Development

No branches or pull requests

3 participants