Renaming SablierV2Config to SablierV2Base

[PaulRBerg]

@andreivladbrg - WDYT about this?

[PaulRBerg]

@andreivladbrg would love your thoughts on this idea!

I think SablierV2Base sounds much better than SablierV2Config, and it will be possible to continue using this name when we develop additional contracts in V2.x. The interface will change, but it will remain backward-compatible.

[andreivladbrg]

So I would be inclined to say that your definition of "actions" is inaccurate

I don't think you got my point, my definition is accurate, the fact there is one constant function doesn't say anything about the fact that the actions(functions that modify state) in that contract are callable only by the admin.

Yes but that does not change the fact that there are functions that can be called by everyone else

There is only one "inoffensive" function that can even be removed if we make the _protocolRevenues mapping public. Similar thing we've done on PRBProxy.

[PaulRBerg]

the fact there is one constant function doesn't say anything about the fact that the actions(functions that modify state) in that contract are callable only by the admin.

I agree. But you keep missing my point - which is that word games are worthless. What matters is substance. And in this case, the substance is that there's more logic in the SablierV2Config contract, logic which can be accessed by anyone.

There is only one "inoffensive" function

Wrong, there is also MAX_FEE and comptroller. Public storage variables have constant functions attached to them.

[PaulRBerg]

if we make the _protocolRevenues mapping public

Great idea, I'll create an issue for it.

[andreivladbrg]

you keep missing my point

You also keep missing the point I am making: The visibility of the storage doesn't matter as long as it can be modified only by the admin.

the substance is that there's more logic in the SablierV2Config contract, logic which can be accessed by anyone.

Ofc, variables that are used for creating the state that can be modified only by the admin.

[PaulRBerg]

You also keep missing the point I am making

We cannot both miss each other's point in this conversation. One of us is wrong. And I have explained quite clearly using various terminology why I think that I'm not the one wrong here.

Anyway, we deviated into word games, which I repeat - they are worthless.

[PaulRBerg]

Summoning @razgraf to help us make a decision here.

[razgraf]

---

Joke aside, my 2c: SablierV2Base sounds a bit misleading. For me the Base of the (lockup part of the) protocol is rather SablierV2Lockup because it defines a core set of streaming concepts.

SablierV2Config defines a set of plug-in functionalities, non-related to streaming itself but to the configuration of the contracts (fees, admins etc.). I'll exaggerate a bit here but the protocol would work fine even without this contract, therefore it cannot be considered a base/foundational contract.

Tl;dr; I find config a much better suited name than base.

[PaulRBerg]

SablierV2Lockup is absolutely not the base in the wider context of Sablier V2 - we have to take into account future contracts!

SablierV2Base (formerly SablierV2Config) is the true base, because it will underpin not just the SablierV2 lockup contracts, but all future contracts.

[PaulRBerg]

Also see my point below about the comptroller being the true "configurator" of the protocol.

[razgraf]

is absolutely not the base in the wider context of Sablier V2

I have specifically said "(lockup part of the)" to assure you I'm not talking about the entire protocol, but simply about lockup as an example.

SablierV2Base (formerly SablierV2Config) is the true base

I've shared my thoughts in that comment as to why I don't consider it being the "base", even though it sits at a very base level in our inheritance schema.

[PaulRBerg]

I have just found my best argument in favor of Base over Config:

The comptroller is the one in charge of protocol configuration! This is literally how we define it in the NatSpec comments:

https://github.com/sablierhq/v2-core/blob/69490679f55d9a245a30a1dba7e96f6e27f162b2/src/interfaces/ISablierV2Comptroller.sol#L10-L11

Having two contracts defined using similar terms is not helpful, so I suggest we crack on with my proposal to use SablierV2Base.

Including this naming in V2.0 would also provide a neat way to add additional logic while maintaining backward compatibility.

[andreivladbrg]

Totally agree with @razgraf here

[PaulRBerg]

the entity in charge

in charge of configuration*

The config is the setup through which a comptroller will perform its duties

I'm sorry but this doesn't make sense.

The comptroller doesn't perform its duties through any setup - there is no channel of communication between the comptroller and the rest of the protocol. Quite the opposite - the arrow of communication is from the protocol to the comptroller.

I still believe "Base" is an unclear choice

You may have simply gotten used to the "Config" wording (and so did @andreivladbrg).

I urge you - and @andreivladbrg - to consider the long-term prospect of being able to keep using the same interface name (ISablierV2Base). That is, we will be able to stuff more logic into this interface e.g. if and when the open-ended streaming contract is developed. For example, the getSender and getRecipient functions would be shared in this base interface.

[PaulRBerg]

Any follow-up here, guys?

I have slept on this topic, and I still like SablierV2Base better. I still think that my point is sound, i.e. there is no config "through which" the comptroller does its job. The comptroller is the config.

[andreivladbrg]

Disclaimer: my last comments on this topic.


I still believe that my points, as well as @razgraf's, may have been misunderstood by you.

The contract is designed to be controlled only by the admin(person who sets up the configuration), the visibility of its storage does not matter.

There can be multiple instances of the comptroller, but if the admin sets the storage variable as address(0), that's considered the "configuration". In other words, there can be a configuration without an actual comptroller involved. The SablierV2Config is the final source of truth, and it will determine if a certain SablierV2Comptroller is actually in charge or not.

The comptroller is the config.

As said above, it can be considered a "config" only if it is explicitly set to be so.

What I believe @razgraf's point about "Base" being unclear in this context is that a contract with this name should actually implement functions related to streaming, rather than just relating on the inheritance schema.

to consider the long-term prospect of being able to keep using the same interface name (ISablierV2Base)

A fee system should not be a requirement for all contracts, streaming can be done without it, although it may be used for other products. Instead, ISablierV2Base should include functions like getSender, getRecipient, cancel, withdraw, and it can also inherit ISablierV2Config if desired.

[PaulRBerg]

Disclaimer: my last comments on this topic.

Is this because of the pressure to freeze the repo for audit?

the visibility of its storage does not matter.

I don't see the logic of why it does not matter. Public storage variables:

1. Are part of the contract ABI
2. Can be seen publicly on explorers like Etherscan
3. They can be called like functions, because they are functions

There can be multiple instances of the comptroller, but if the admin sets the storage variable as address(0), that's considered the "configuration"

This is entirely orthogonal to my point.

Under my explanation, all of those comptrollers would be configurations themselves. I mean, if setting the fees does not mean the configuration, I don't know what configuration means anymore!

There's also the action of setting the configuration itself. Still, my point is to resort to an economy of terms and not re-use the term "configuration" for the meta action of configuring the configuration itself.

a contract with this name should actually implement functions related to streaming,

yes, and it will, eventually! I mentioned five times above that one of the goals of this contract is to make it possible to retroactively add interfacing logic while keeping the V2.0 interface name intact.

A fee system should not be a requirement for all contracts, streaming can be done without it

Au contraire!!

SablierV2 is inevitably tied to a fee system. There's the broker parameter and the fee calculations scattered around the code base, e.g. in the Helpers library.

This is why it makes complete sense to have the setComptroller, protocolRevenues, and claimProtocolRevenues together make up the SablierV2Base contract (I repeat: we will be able to extend this with the functions you mentioned, getSender, etc.)

[PaulRBerg]

Implemented via #377.

[razgraf]

You asked for feedback on this but it doesn't look like you've truly considered it before merging 👍

[PaulRBerg]

@razgraf you didn't have context on this.

I had a private discussion with @andreivladbrg that today I will work on everything on my own, merge everything without review in advance, and then @andreivladbrg will provide review on the final result tomorrow.

We're in an exceptional situation - not much time for debates.

[PaulRBerg]

Though I will concede that it wasn't polite to ask you for feedback, and then merge that PR right away and close this discussion (I just reopened it now).

Sorry for this. I've worked too much lately; I feel exhausted.

[PaulRBerg]

Going to close this since the finalized version of the repo (ready for audit) uses SablierV2LockupBase.