Any reason for not ordering the ConstructorParams alphabetically?

[PaulRBerg]

Here:

v2-periphery/src/types/DataTypes.sol

Lines 101 to 110 in 06ffb31

	struct ConstructorParams {
	address initialAdmin;
	IERC20 asset;
	string ipfsCID;
	string name;
	bytes32 merkleRoot;
	uint40 expiration;
	bool cancelable;
	bool transferable;
	}

Cc @sablier-labs/solidity.

[smol-ninja]

The same reason why we don't have them alphabetically ordered in other create structs in DataTypes i.e. similar field types are put next to each other.

 struct ConstructorParams { 
     address initialAdmin; 
     IERC20 asset; 
     string ipfsCID; 
     string name; 
     bytes32 merkleRoot; 
     uint40 expiration; 
     bool cancelable; 
     bool transferable; 
 }

vs

 struct ConstructorParams { 
     IERC20 asset;
     bool cancelable; 
     uint40 expiration; 
     address initialAdmin; 
     string ipfsCID;
     bytes32 merkleRoot;  
     string name; 
     bool transferable; 
 } 

If we order them alphabetically then we should follow the same pattern for other memory structs too. Isn't?

[PaulRBerg]

The problem is that the ConstructorParams has grown over time and having to choose where to insert the next new param is a chore (it also leads to debates in PRs). Alphabetical ordering would enable a non-opinionated design.

But yeah, you're right that if we do this, we should do it universally for all memory structs. And the problem with that is that it would lead to a major refactor, for which we don't really have time now. Maybe we can include this in the next release when we switch to package tethering? We should open an issue.

[smol-ninja]

Agree with you. I don't agree to re-order it due to the reasons shared by andrei and razgraf.

[andreivladbrg]

I don't agree with this, I think this comment left by you @PaulRBerg is still relevant here: sablier-labs/v2-core#784 (comment)

I consider the current version of the order of core structs parameters better, especially because:
"The reason for this is that it would (ironically) create more consistency among our create function calls - across the production code in src, the test code in test, and our examples"

Also tagging @razgraf as I remember that he mentioned that constantly changing the order of the parameters in structs would increase the complexity of the app, since we need to maintain the backwards compatibility.

[smol-ninja]

Yes. I forgot about our previous discussion. Thanks for sharing it again Andrei. I don't agree with re-order too as it changes the API and the alphabetical re-order doesn't increase the readability of the struct very much.

[smol-ninja]

Issues opened:

• Refactor memory structs to have parameters in an alphabetical order v2-core#888
• Refactor memory structs to have parameters in an alphabetical order #316

[razgraf]

Original reply from sablier-labs/v2-core#888 (comment):

I'm against re-ordering as well. The advantage of a slightly cleaner/non-opinionated struct here is too small to make the time spent refactoring our utilities worth it.

See this backwards compatibility code (nightmare) we have to write in order to maintain all versions of the protocol inside the subgraph. This extends to indexers, app etc.

As long as (1) there is no functional advantage (optimization, heavy gas reduction, security or logic) or (2) impressive cognitive load reduction / confusion reduction as far as DX goes, I please urge you to re-consider this type of refactors. I myself find it hard to keep track of these re-namings and re-orderings, cannot imagine what an external integrator has to go through.

[PaulRBerg]

I made a mistake by asking you to repost your comments in this discussion, @razgraf, @andreivladbrg. Sorry for that.

There are two ideas intertwined in this thread:

1. Alphabetically order the parameters in the newly introduced ConstructorParams struct (part of V2.2)
2. Alphabetically order the parameters in all memory structs

Andrei and Razvan's criticism is valid for the 2nd idea, but not the 1st:

• There's no path dependence with the ConstructorParams struct because we're introducing it now for the first time in production
• None of the arguments I made here apply to ConstructorParams
• The current ordering isn't great - why does ipfsCID before merkleRoot? And why is name in between?
  • Alphabetical ordering would 'cut across' these opinionated questions

Given the rationale above, I went ahead and made a PR to order the fields alphabetically in this particular struct:

#326

[andreivladbrg]

yes, this is why i've specifically written that message in the core issue and not here

i agree with the rationale behind ConstructorParams