feat: membership

[richard-ramos]

Description

Adds membership management as described in #11 and https://github.com/waku-org/specs/blob/master/standards/core/rln-contract.md .

Some test units are still pending but the contract is already in a state in which comments on it can be made.
I added some observations too about some points in the contract which require clarification.

Some changes regarding the spec were made based on comments on #11 as well as removing the need of having an actual membership state stored, since this can be determined based on the expiration dates and existence of the membership in the contract.

To track the membership age I implemented this as a double linked list. It's worth mentioning that keeping track of the expired memberships means some gas expense due to having to maintain the linking for the memberships in this list. I'm exploring whether using array that grows infinitely as a list and keeping track of the index in which to start navigating the array, to see if I can save some gas this way. Suggestions of other data structures to keep track of this information are appreciated, although maybe we could consider not keeping track of the oldest memberships and have this indexing being done offchain.

Tomorrow I expect to finish the pending test units based on this implementation.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

• Added natspec comments?
• Ran pnpm adorno?

[richard-ramos]

Besides the limit and number of periods, is there any other information we might need in the future to calculate the price? currently only these items are needed for linear price calculation

[richard-ramos]

For nwaku, it would only require the index, right? I added the idCommitment and userMessageLimit in case we want to calculate the rateCommitment on the nwaku side, but is it really necessary?

[alrevuelta]

index should be enough but not sure if the fact that memberships expire may require idcommit/msglimit. I mean, I don't think it would be strictly necessary but nice for checking that the index you are removing corresponds to the commitment you are expecting. But yeah, a nice to have, not required.

[richard-ramos]

This does not take into account the expired memberships, although since it's an external function I could attempt to optionally calculate whether the rate limit of currently expired memberships would allow the userMessageLimit to be registered. WDYT?

[richard-ramos]

Having the price calculation being done on a separate contract means that if in the future we want to move away from linear calculation to a curve, it should be possible by just changing the current priceCalculator used by the Membership contract

[richard-ramos]

This will try to remove expired memberships sequentially, but it's possible that the accumulated rate limit of all the memberships being removed is not enough for the chosen rate limit to acquire. Should we also accept an optional list of memberships to expire, so the user of the contract can explore the contract offchain and pass a list expired memberships whose accumulated rate limits is enough to acquire the rate limit they need?

[richard-ramos]

Something interesting here is to prefer inserting new elements on the tree rather than attempt to erase expired memberships first, mainly because there's no guarantee that an expired membership rate limit will match the rate limit to acquire. (and then having to iterate to obtain enough rate limit from expired memberships).

[s-tikhomirov]

there's no guarantee that an expired membership rate limit will match the rate limit to acquire

Is it technically possible to track / predict how much expired rate limit capacity we will have at a given point in time? (So that we can quickly tell if it's even worth it to start iterating through expired membership in an attempt to overwrite them, or their rate limit won't suffice anyway.)

[fryorcraken]

A proposal we discussed in a call was that:

• optional "slot to override" parameter when doing the insertion
• if no slot to override specified, then it tries to grab next available slot in tree. If global rate limit gets exceeded, it fials
• if slot to override is specified:
  • if slot to override is not over grace period, fails
  • if slot to override rate limit < rate limit of new membership, fails
    Otherwise, replace slot.

This means that in terms of gas cost, it's more predictable:

• there is space, low gas cost
• no space, most specific membership to override, higher gas cost

than the previous idea of "override if there is something to override, otherwise do not".

Also, we should have a call on contract to purge memberships over grace period. so we can be the one to cover the purge cost.

[s-tikhomirov]

if no slot to override specified, then it tries to grab next available slot in tree

I'd like to emphasize here that we have (at least) two types of limited resource:

1. contract storage space;
2. total rate limit.

The idea of slot overwriting was initially meant to save contract storage space. Later on, a question emerged (in my head at least): is saving this particular resource worth the increased gas costs? Agressively overwriting old memberships would definitely consume more gas (at the expense of the user who registers a new membership), whereas savings from a smaller tree are a) not guaranteed; b) spread out across everyone; c) not even that large (?) (I'm not sure how to qunatify it).

TLDR: do we need to

grab next available slot in tree

every time, or is it perhaps better to do this (more) lazily?

[richard-ramos]

Is it technically possible to track / predict how much expired rate limit capacity we will have at a given point in time? (So that we can quickly tell if it's even worth it to start iterating through expired membership in an attempt to overwrite them, or their rate limit won't suffice anyway.)

I'd say this can be done offchain and then via function parameters indicate to the contract whether we want to: just insert a new leaf in the tree / have the contract remove automatically expired / remove specific memberships. Then nwaku could call estimateGas on any of these options and choose the one that incurs in less gas cost.

A proposal we discussed in a call ...

Ah! this seems to be aligned with what I just mentioned

TLDR: do we need to grab next available slot in tree every time, or is it perhaps better to do this (more) lazily?

This is hard to answer without testing. Perhaps we can have the three options I mentioned earlier, and have some script to try to determine the behavior of the contract once we have a large enough number of memberships.

[richard-ramos]

@fryorcraken, @s-tikhomirov ,
In bf757e7 the register function was modified and now exists as two overloaded functions:

register(uint256 idCommitment, uint32 userMessageLimit);
register(uint256 idCommitment, uint32 userMessageLimit, uint256[] calldata membershipsToErase)

I'm still working on the test units for the last function but this should cover what was discussed before.

[fryorcraken]

The idea of slot overwriting was initially meant to save contract storage space

Interesting, for me it was about booting off the network publishers with expired membership to keep network traffic and membership set closely related.

every time, or is it perhaps better to do this (more) lazily?

As @richard-ramos ' stated:

register(uint256 idCommitment, uint32 userMessageLimit);
register(uint256 idCommitment, uint32 userMessageLimit, uint256[] calldata membershipsToErase)

With 2 functions on the contrat, it is now possible for the app to provide more feedback:

1. App sees there is space on the tree (R_{max} not reached yet, including new membership).
2. App can use register(uint256 idCommitment, uint32 userMessageLimit); and only an insertion happen

else

1. App sees there is no space on the tree (R_{max} is reached when including new membership).
2. App finds an Expired membership with greater or equal rate limit (a good actor would actually use the expired membership with greatest rate limit)
3. App can warn user that membership insertion would cost a bit more than usual
4. Insertion can be done with register(uint256 idCommitment, uint32 userMessageLimit, uint256[] calldata membershipsToErase)

This gives more predictability to the user.
App devs can also decide to spend the gas to purge Expired memberships to make it cheaper for users.

[s-tikhomirov]

Left a few comments, will look into the code in more detail next week, thank you!

One meta-point I'd like to make for now: I think we should discuss and make a decision on @alrevuelta's proposal about billing periods. If we conclude that this is a good idea, then I think we should first amend the spec to reflect these changes, and then review the code to ensure it matches the updated spec.

[alrevuelta]

Left some comments. I'm a bit concerned with the x2 gas increase.

[alrevuelta]

a valid registration seems to go from 124408 to 268741 of gas, which is x2. Can we optimize it?

It's worth mentioning that keeping track of the expired memberships means some gas expense due to having to maintain the linking for the memberships in this list

Ideally the contract should be as simple as possible, and I would say that's possible but we may have to circle back to the spec. Perhaps some things can be handled offchain, or well, its state does not need to be tracked onchain.

[richard-ramos]

I can try optimizing it, but the main problem IMO is the additional storage required due to

waku-rlnv2-contract/src/Membership.sol

Lines 39 to 96 in 251d890

	/// @notice Maximum total rate limit of all memberships in the tree
	uint32 public maxTotalRateLimitPerEpoch;
	/// @notice Maximum rate limit of one membership
	uint32 public maxRateLimitPerMembership;
	/// @notice Minimum rate limit of one membership
	uint32 public minRateLimitPerMembership;
	/// @notice Membership billing period
	uint32 public billingPeriod;
	/// @notice Membership grace period
	uint32 public gracePeriod;
	/// @notice balances available to withdraw
	mapping(address holder => mapping(address token => uint256 balance)) public balancesToWithdraw;
	/// @notice Total rate limit of all memberships in the tree
	uint256 public totalRateLimitPerEpoch;
	/// @notice List of registered memberships
	mapping(uint256 idCommitment => MembershipInfo member) public members;
	/// @notice The index of the next member to be registered
	uint32 public commitmentIndex;
	/// @notice track available indices that are available due to expired memberships being removed
	uint32[] public availableExpiredIndices;
	/// @dev Oldest membership
	uint256 public head = 0;
	/// @dev Newest membership
	uint256 public tail = 0;
	struct MembershipInfo {
	/// @notice idCommitment of the previous membership
	uint256 prev;
	/// @notice idCommitment of the next membership
	uint256 next;
	/// @notice amount of the token used to acquire this membership
	uint256 amount;
	/// @notice numberOfPeriods
	uint32 numberOfPeriods;
	/// @notice timestamp of when the grace period starts for this membership
	uint256 gracePeriodStartDate;
	/// @notice duration of the grace period
	uint32 gracePeriod;
	/// @notice the user message limit of each member
	uint32 userMessageLimit;
	/// @notice the index of the member in the set
	uint32 index;
	/// @notice address of the owner of this membership
	address holder;
	/// @notice token used to acquire this membership
	address token;
	}

We could probably cut back the cost by not mainainting the ordered membership list, and have nwaku be the one tracking the age of the memberships and deciding then which memberships to erase. I'd actually prefer that since we make the contract cheaper for the end user, but I don't know if keeping track of the membership age in nwaku is something we want to do?

[fryorcraken]

We could probably cut back the cost by not mainainting the ordered membership list, and have nwaku be the one tracking the age of the memberships and deciding then which memberships to erase. I'd actually prefer that since we make the contract cheaper for the end user, but I don't know if keeping track of the membership age in nwaku is something we want to do?

do you mean that nwaku would need to listen to event to know age?

Is the issue that the list is ordered on the contract (I am not sure which piece of code you are referring with "due to")?

Why ordering the list?

[richard-ramos]

do you mean that nwaku would need to listen to event to know age?

Yes, if we dont want to automatically erase expired memberships, nwaku could instead listen to MemberRegistered events (which should be modified to include the expirationDate, as well as listening to MemberExtended events. Then populate some persisted structure that would allow it to determine which memberships are expired (this can be done for example by using zerokit's set_metadata function, a sqlite table, etc). The advantage of doing this client side is that membership registration can be smarter and choose not just the oldest expired membership, but the minimum amount of expired memberships whose total sum of rate limit is enough to allow registering a new membership.

Is the issue that the list is ordered on the contract (I am not sure which piece of code you are referring with "due to")? Why ordering the list?

According to the specs, one of the requirements specifies The new membership SHOULD overwrite the membership that has been Expired for the longest time . This means that the smart contract should track the age of the memberships, and since sorting the memberships by expiry date is impossible due to gas costs, the solution I found was to use a double linked list in which the head of the list would always point to the oldest membership, with new memberships being added at the end (as well as extended memberships being moved to the tail of the list as well since they'd become the newest.

[fryorcraken]

The new membership SHOULD overwrite the membership that has been Expired for the longest time

I think it would make sense to drop this spec requirement and move to a model where the caller can pass a membership index to overwrite.

Ideally, we should store expiry time (or registration time + length) on the smart contract so we don't need to listen to events.

[jm-clius]

Great work!

@kaiserd @0x-r4bbit could we get a review from the Smart Contract team as well?

[0x-r4bbit]

Looking into this now

[0x-r4bbit]

Left some comments.

Generally, you could make use of custom errors to save some gas

[0x-r4bbit]

In line: 9 we say that address(0) represents ETH as token.
Yet, here we're requiring _token != address(0).

Based on the comment, we either accept address(0) or adjust the comment.

[0x-r4bbit]

Also, to save gas we should make this a custom error instead (they are always 4 byte in size)

[richard-ramos]

In line: 9 we say that address(0) represents ETH as token.
Yet, here we're requiring _token != address(0).
Based on the comment, we either accept address(0) or adjust the comment.

I forgot to delete this when removing the support for ETH

[0x-r4bbit]

Suggestion: There's also Ownable2Step which requires a new owner to accept ownership. This can help to avoid changing ownership to a wrong address.

[richard-ramos]

Sounds good! I'll apply this to the WakuRlnV2 contract as well!

[0x-r4bbit]

Another suggestion: Have MemberShip extend Initializable and then add onlyInitalizing modifier to this function. This ensure the init function can only be called during initialization

[0x-r4bbit]

Minor suggestion: Rename to MembershipUpgradeable

[0x-r4bbit]

Also, would it make sense to make it abstract? I don't think this is meant to be instantiated by itself, is it?

[0x-r4bbit]

There's no input checks on this init function because they are done in WakuRlnV2 contract.

I'd consider moving them here.

[0x-r4bbit]

I'm curious: in which case would msg.value > 0 be true? It doesn't look like we ever call _transferFees() from a payable function.

[0x-r4bbit]

Do you think it makes sense to first check whether the retrieved MembershipInfo is valid? Inc ase idCommitmentToErase is a faulty one.

Otherwise it looks like this will be implicitly checked through things like grace period and expiry, correct?

[richard-ramos]

Otherwise it looks like this will be implicitly checked through things like grace period and expiry, correct?

Hm. Good point. It's probably better to be explicit in this case.

[richard-ramos]

Thank you for the review, @0x-r4bbit. I fixed the items and included your suggestion in the last commit

[s-tikhomirov]

It looks like this is the only usage of the function idCommitmentToMetadata, and we ignore two out of three values it returns. Would it be more efficient to implement something like getRateCommitment instead?