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

feat: membership #13

Merged
merged 21 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions .gas-snapshot
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
WakuRlnV2Test:test__IdCommitmentToMetadata__DoesntExist() (gas: 16726)
WakuRlnV2Test:test__InvalidPaginationQuery__EndIndexGTcommitmentIndex() (gas: 18249)
WakuRlnV2Test:test__InvalidPaginationQuery__StartIndexGTEndIndex() (gas: 16130)
WakuRlnV2Test:test__InvalidRegistration__DuplicateIdCommitment() (gas: 99502)
WakuRlnV2Test:test__InvalidRegistration__FullTree() (gas: 14328)
WakuRlnV2Test:test__InvalidRegistration__InvalidIdCommitment__LargerThanField() (gas: 15229)
WakuRlnV2Test:test__InvalidRegistration__InvalidIdCommitment__Zero() (gas: 14004)
WakuRlnV2Test:test__InvalidRegistration__InvalidUserMessageLimit__LargerThanMax() (gas: 17703)
WakuRlnV2Test:test__InvalidRegistration__InvalidUserMessageLimit__Zero() (gas: 14085)
WakuRlnV2Test:test__Upgrade() (gas: 3791109)
WakuRlnV2Test:test__ValidPaginationQuery(uint32) (runs: 1000, μ: 445155, ~: 159972)
WakuRlnV2Test:test__ValidPaginationQuery__OneElement() (gas: 119773)
WakuRlnV2Test:test__ValidRegistration(uint256,uint32) (runs: 1000, μ: 124408, ~: 124408)
WakuRlnV2Test:test__ValidRegistration__kats() (gas: 96616)
WakuRlnV2Test:test__IdCommitmentToMetadata__DoesntExist() (gas: 27478)
WakuRlnV2Test:test__InsertionNormalOrder(uint32) (runs: 1001, μ: 1123469, ~: 494837)
WakuRlnV2Test:test__InvalidPaginationQuery__EndIndexGTcommitmentIndex() (gas: 18261)
WakuRlnV2Test:test__InvalidPaginationQuery__StartIndexGTEndIndex() (gas: 16108)
WakuRlnV2Test:test__InvalidRegistration__DuplicateIdCommitment() (gas: 249370)
WakuRlnV2Test:test__InvalidRegistration__InvalidIdCommitment__Zero() (gas: 12135)
WakuRlnV2Test:test__Upgrade() (gas: 6728616)
WakuRlnV2Test:test__ValidPaginationQuery(uint32) (runs: 1002, μ: 226428, ~: 52927)
WakuRlnV2Test:test__ValidPaginationQuery__OneElement() (gas: 262351)
WakuRlnV2Test:test__ValidRegistration(uint256,uint32) (runs: 1001, μ: 268741, ~: 268741)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// @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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@richard-ramos richard-ramos Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

WakuRlnV2Test:test__ValidRegistration__kats() (gas: 238744)
2,459 changes: 1,371 additions & 1,088 deletions pnpm-lock.yaml

Large diffs are not rendered by default.

8 changes: 6 additions & 2 deletions script/Deploy.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,22 @@
pragma solidity >=0.8.19 <=0.9.0;

import { WakuRlnV2 } from "../src/WakuRlnV2.sol";
import { LinearPriceCalculator } from "../src/LinearPriceCalculator.sol";
import { PoseidonT3 } from "poseidon-solidity/PoseidonT3.sol";
import { LazyIMT } from "@zk-kit/imt.sol/LazyIMT.sol";
import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";

Check warning on line 9 in script/Deploy.s.sol

View workflow job for this annotation

GitHub Actions / lint

imported name UUPSUpgradeable is not used

import { BaseScript } from "./Base.s.sol";
import { DeploymentConfig } from "./DeploymentConfig.s.sol";

contract Deploy is BaseScript {
function run() public broadcast returns (WakuRlnV2 w, address impl) {
// TODO: Use the correct values when deploying to mainnet
address priceCalcAddr = address(new LinearPriceCalculator(address(0), 0.05 ether));
// TODO: set DAI address 0x6B175474E89094C44Da98b954EedeAC495271d0F
impl = address(new WakuRlnV2());
bytes memory data = abi.encodeCall(WakuRlnV2.initialize, 100);
bytes memory data = abi.encodeCall(WakuRlnV2.initialize, (priceCalcAddr, 160_000, 20, 600, 30 days, 5 days));
// (priceCalcAddr, 160000, 20, 600, 30 days, 5 days)
address proxy = address(new ERC1967Proxy(impl, data));
w = WakuRlnV2(proxy);
}
Expand All @@ -22,12 +26,12 @@
contract DeployLibs is BaseScript {
function run() public broadcast returns (address poseidonT3, address lazyImt) {
bytes memory poseidonT3Bytecode = type(PoseidonT3).creationCode;
assembly {

Check warning on line 29 in script/Deploy.s.sol

View workflow job for this annotation

GitHub Actions / lint

Avoid to use inline assembly. It is acceptable only in rare cases
poseidonT3 := create(0, add(poseidonT3Bytecode, 0x20), mload(poseidonT3Bytecode))
}

bytes memory lazyImtBytecode = type(LazyIMT).creationCode;
assembly {

Check warning on line 34 in script/Deploy.s.sol

View workflow job for this annotation

GitHub Actions / lint

Avoid to use inline assembly. It is acceptable only in rare cases
lazyImt := create(0, add(lazyImtBytecode, 0x20), mload(lazyImtBytecode))
}
}
Expand Down
11 changes: 11 additions & 0 deletions src/IPriceCalculator.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;

interface IPriceCalculator {
richard-ramos marked this conversation as resolved.
Show resolved Hide resolved
/// Returns the token and price to pay in `token` for some `_rateLimit`
/// @param _rateLimit the rate limit the user wants to acquire
/// @param _numberOfPeriods the number of periods the user wants to acquire
/// @return address of the erc20 token
/// @return uint price to pay for acquiring the specified `_rateLimit`
function calculate(uint256 _rateLimit, uint32 _numberOfPeriods) external view returns (address, uint256);
richard-ramos marked this conversation as resolved.
Show resolved Hide resolved
}
36 changes: 36 additions & 0 deletions src/LinearPriceCalculator.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;

import { Ownable } from "openzeppelin-contracts/contracts/access/Ownable.sol";
import { IPriceCalculator } from "./IPriceCalculator.sol";

/// @title Linear Price Calculator to determine the price to acquire a membership
contract LinearPriceCalculator is IPriceCalculator, Ownable {
richard-ramos marked this conversation as resolved.
Show resolved Hide resolved
/// @notice Address of the ERC20 token accepted by this contract. Address(0) represents ETH
address public token;

/// @notice The price per message per epoch per period
uint256 public pricePerMessagePerPeriod;

constructor(address _token, uint256 _pricePerPeriod) Ownable() {
token = _token;
pricePerMessagePerPeriod = _pricePerPeriod;
}

/// Set accepted token and price per message per epoch per period
/// @param _token The token accepted by the membership management for RLN
/// @param _pricePerPeriod Price per message per epoch
function setTokenAndPrice(address _token, uint256 _pricePerPeriod) external onlyOwner {
token = _token;
pricePerMessagePerPeriod = _pricePerPeriod;
}

/// Returns the token and price to pay in `token` for some `_rateLimit`
/// @param _rateLimit the rate limit the user wants to acquire
/// @param _numberOfPeriods the number of periods the user wants to acquire
/// @return address of the erc20 token
/// @return uint price to pay for acquiring the specified `_rateLimit`
function calculate(uint256 _rateLimit, uint32 _numberOfPeriods) external view returns (address, uint256) {
return (token, uint256(_numberOfPeriods) * uint256(_rateLimit) * pricePerMessagePerPeriod);
}
}
Loading
Loading