generated from vacp2p/foundry-template
-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: membership #13
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
f3d085d
feat: membership
richard-ramos fe01893
chore: more test units and fixes
richard-ramos 5ddfc35
chore: more test units and fixes
richard-ramos 9a1c1dc
refactor: minor optimization when expiring memberships
richard-ramos 9251400
chore: some fixes and gas optimizations
richard-ramos a369de9
chore: more tests
richard-ramos 0d2f43e
fix: fulltree test
richard-ramos f91269a
fix: lint
richard-ramos 251d890
chore: more tests
richard-ramos 0cd80d6
refactor: use expiration term instead of billing period
richard-ramos 7f60c53
fix: remove ETH support
richard-ramos 6ba9a5b
fix: lint
richard-ramos c66f728
fix: type for rate limit in price calculator
richard-ramos 12e837a
feat: allow choosing automatic membership erases and specific members…
richard-ramos 22ddb35
chore: more test units
richard-ramos a658315
chore: rename `commitmentIndex` to `nextCommitmentIndex`
richard-ramos 3fbcbfe
chore: update readme and script to allow running on anvil
richard-ramos df502c7
fix: code review
richard-ramos 522599c
fix: more efficient duplicate members check
richard-ramos 3fea0a5
refactor: do not keep track of membership registration order (#14)
richard-ramos e8ee1d7
feat: `Ownable2StepUpgradeable` to `OwnableUpgradeable` (#21)
richard-ramos File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) | ||
WakuRlnV2Test:test__ValidRegistration__kats() (gas: 238744) |
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
to268741
of gas, which is x2. Can we optimize it?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.
There was a problem hiding this comment.
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
waku-rlnv2-contract/src/Membership.sol
Lines 39 to 96 in 251d890
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?
There was a problem hiding this comment.
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?
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toMemberExtended
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'sset_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.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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.