Codebase feedback and questions (Max, March '23)

[maxdesalle]

Some minor suggestions and questions that came to mind while going through the contracts:

• Shouldn't we replace the "V2" in the ascii art with "V2.0" given there will be new versions of the protocol?
• Is it normal that the license for the SablierV2NftDescriptor.sol contract is unlicensed, as opposed to licensed with the BUSL-1.1? Same for the interface contracts, they are all licensed under the GPL.
• Very minor thing, but it seems like there is unnecessary spacing here (and in the below function descriptions too).
• Why is there a limit on the fee frontends can charge?

[andreivladbrg]

Shouldn't we replace the "V2" in the ascii art with "V2.0" given there will be new versions of the protocol?

That's a good point.

Is it normal that the license for the SablierV2NftDescriptor.sol contract is unlicensed

That contract is just an example used for scripts and tests. The actual implementation will be under BUSL.

Same for the interface contracts, they are all licensed under the GPL.

All the interfaces are left under the GPL because they are meant to be imported by our integrators. The actual implementation of them is under BUSL.

it seems like there is unnecessary spacing here

The spacing between natspecs and comments has been intentionally left on all functions with longer documentation to improve readability.

Why is there a limit on the fee frontends can charge?

I will let @PaulRBerg answear this question.

[PaulRBerg]

Excellent feedback; thank you, @maxdesalle!

Shouldn't we replace the "V2" in the ascii art with "V2.0" given there will be new versions of the protocol?

It's an interesting observation. On the one hand, I would be inclined to say that we should keep "V2" in the ASCII art, since "V2" implicitly encompasses all versions (V2.0, V2.1, etc). On the other hand, using the dot notation would be more specific.

The catch with on-chain-specific versions is that we may only update some contracts on a subsequent new release. For instance, we could ship SablierV2Comptroller without touching any of the SablierV2Lockup contracts. Wouldn't it be somewhat confusing to have an on-chain system that represents our "V2.1" release, but which comprises contracts that contain "V2.0" in their ASCII art?

Let's sleep on this idea before making a decision.

Is it normal that the license for the SablierV2NftDescriptor.sol contract is unlicensed

@andreivladbrg answered this.

We will have the final implementation for the NFT descriptor only in April after we receive the visual assets from the graphic designers.

Same for the interface contracts, they are all licensed under the GPL

Again, @andreivladbrg answered this. There's more context on the licensing in the README.

there is unnecessary spacing here

That spacing there is intentional. It serves as a visual delineator between the @notice and the @dev tags. The former is a simple explainer of what the function does, which is meant to be understood by everyone; the latter is a more advanced description aimed at developers.

Why is there a limit on the fee frontends can charge?

A host of reasons:

1. Gas optimization. It is easier to check for MAX_FEE (the same cap applied to protocol fees) than check for a bespoke 100% cap in the case of broker fees (note: the broker fee must not be greater than 100%, lest the ERC-20 transfer fails and other protocol assumptions break)
2. Security. Having a cap prevents accidental decimal miscalculations.
3. A MAX_FEE of 10% should be enough for the vast majority of use cases (10% is what we believe MAX_FEE will be). If any hungry capitalist frontend wants to charge more than 10%, they could charge that high fee at their frontend level, and see the broker.fee to 0% in the create stream transaction.

[PaulRBerg]

I'd like to hear @razgraf's thoughts on the ASCII versioning issue, since this is, in part, a matter of product presentation.

[razgraf]

While referring to it as v2.0 is clearer, for the sake of simplicity I'd just keep V2 there (with the possibility of adding a smaller semver versioning number somewhere underneath).

The split-deployment argument where only pieces are upgraded, not the entire protocol makes a good case for the "V2" naming.

[PaulRBerg]

Awesome, thanks for confirming.

A hardcoded semantic version would indeed be nice! Similar to the one I have in PRBProxy.

[PaulRBerg]

Closing, as we've settled and/ or responded to all points.