Add Potlock contract #76
Open
Conversation
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
sasurobert
reviewed
Jun 17, 2024
dorin-iancu
reviewed
Jun 26, 2024
| fn project_donations( | ||
| &self, | ||
| project_id: ProjectId, | ||
| ) -> MapMapper<ManagedAddress, EsdtTokenPayment>; |
There was a problem hiding this comment.
Unless you've got no other choice, avoid MapMapper.
Comment on lines
42
to
45
| let project_id = self.projects().len() + 1; | ||
| let owner = self.blockchain().get_caller(); | ||
| let project = Project::new(project_id, potlock_id, project_name, description, owner); | ||
| self.projects().push(&project); |
There was a problem hiding this comment.
I don't think this respects the specs. I don't see where an admin/owner can accept/reject an application. Docs say this:
ApplyForPot@potID@projectName@description - anyone can submit application, but an authority will review the submission and evaluate for eligibility and potential impact
There was a problem hiding this comment.
The new function for Project::new will create a Project with Status = Inactive.
An authority would have to call endpoint(acceptApplication) to set the project status to Active
Base automatically changed from
unified-syntax-refactor
to
framework-upgrade-0.50.4
July 2, 2024 14:28
Framework upgrade potlock
Add blackbox tests
Potlock system tests
Coverage SummaryTotals
FilesExpand
|
psorinionut
requested changes
Jul 29, 2024
| ); | ||
| let caller = self.blockchain().get_caller(); | ||
|
|
||
| let potlock_id = self.potlocks().len() + 1; |
There was a problem hiding this comment.
I don't think this is a good idea, as you also have a way to remove data from this storage (removePot endpoint) - which means it may overwrite already existing ids. So it will be better if you keep a separate storage for the lastId and increment that value each time a new pot is created.
There was a problem hiding this comment.
Removing the second element from a VecMapper that contains 4 elements (1, 2, 3, 4), we would get 1, 3, 4.
Removing the last element from a VecMapper that contains 4 elements (1, 2, 3, 4), we would get 1, 2, 3. By adding another element, we would get 1, 2, 3, 5.
There is no need for the lastId.
| let caller = self.blockchain().get_caller(); | ||
| self.require_potlock_exists(potlock_id); | ||
| self.require_potlock_is_active(potlock_id); | ||
| self.pot_donations(potlock_id).insert(caller, payment); |
There was a problem hiding this comment.
There is a problem here if I donate twice. The old donation would be overwritten and I'll remain with that amount in the contract unregistered. There are a few ways around this:
- You take the output of the insert function and send back to the user
- You add a check if there is already a donation on behalf of that user.
- Instead of a EsdtTokenPayment per user, you save a PaymentsVec.
There was a problem hiding this comment.
Accepted multiple payments but only with the same token_id.
There was a problem hiding this comment.
This function is still the same. The payment token is not checked. And in case you only ask for a specific payment token, you would still need a check, to see if there is already some donation from the caller. If it isn't, then you add it, if there's already something, you accumulate it. But this works only if the accepted token_id is always the same and not changeable.
There was a problem hiding this comment.
Now I see you did this update for the donateToProject endpoint, but it is needed here also.
| for pp in project_percentage { | ||
| let (project_id, percentage) = pp.into_tuple(); | ||
| let mut output_payments = ManagedVec::new(); | ||
| for (_, donation) in pot_donations.iter() { |
There was a problem hiding this comment.
There may be a few problems with this way of implementing (as Dorin also suggested), mostly concerning gas usage. If a malicious user donates values from 1000 addresses, the potlock will be blocked, with an outOfGas error.
Maybe rethink the architecture of the storages. One option would be instead of a mapMapper, to save under the potlock_id and user keys, a singleValueMapper of PaymentsVec. This allows you to quickly go to a specific user for donations and also, this endpoint would control the gasCost by clearly providing a list of users, besides the potlockId.
There was a problem hiding this comment.
We are unable to iterated through all payments and distribute them if we have singleValueMapper.
|
|
||
| #[view(getProjects)] | ||
| #[storage_mapper("projects")] | ||
| fn projects(&self) -> VecMapper<Project<Self::Api>>; |
There was a problem hiding this comment.
UnorderedSetMapper here as well.
There was a problem hiding this comment.
Would get a different behavior as explained to potlocks.
Modify require_correct_percentages from distributePotToProjects endpoint. The total may be lower than 100%. This enables the caller to distribute part of the pot.
alyn509
previously approved these changes
Aug 6, 2024
psorinionut
requested changes
Aug 6, 2024
| self.send().direct_multi(&project_owner, &output_payments); | ||
| } | ||
|
|
||
| self.pot_donations(potlock_id).clear(); |
There was a problem hiding this comment.
Because you completely clear (instead of deduct each payment), the require_correct_percentages should check that the total percentage == 100%, instead of <= 100%.
There was a problem hiding this comment.
Also, after the pot is distributed to the projects, it still remains active? So once accepted, the pot remains active indefinitely?
If not, maybe some extra storage clearing should be done for the potlocks mapper?
|
|
||
| #[view(feeAmountAcceptPots)] | ||
| #[storage_mapper("feeAmountAcceptedPots")] | ||
| fn fee_amount_accepted_pots(&self) -> SingleValueMapper<BigUint>; |
There was a problem hiding this comment.
What's the usage of this storage? You only increment it in the acceptPot, so just to keep track of the total pot amounts over time? Do you need this in the SC stored?
| fn add_pot(&self, name: ManagedBuffer, description: ManagedBuffer) { | ||
| let payment_for_adding_pot = self.call_value().single_esdt(); | ||
| require!( | ||
| self.fee_token_identifier().get() == payment_for_adding_pot.token_identifier, |
There was a problem hiding this comment.
Only one payment token for all pots? And an ESDT? So I guess it will be WEGLD?
There was a problem hiding this comment.
This is the token paid by the pot initiator (from the specs: AddPot@potDescription - it sets the description and the name of the pot, the call requires a FEE in a defined tokenID.)
The fee token can be set to any token, probably WEGLD.
Another contract can be deployed for xExchange and the fee will probably be MEX.
There was a problem hiding this comment.
The donations can be in any ESDT token.
|
|
||
| #[only_admin] | ||
| #[endpoint(acceptApplication)] | ||
| fn accept_application(&self, project_id: ProjectId) { |
There was a problem hiding this comment.
Maybe also add another endpoint to reject and remove project applications? Like with the pot?
| let pot_donations = self.pot_donations(potlock_id); | ||
|
|
||
| for pp in project_percentages { | ||
| let (project_id, percentage) = pp.into_tuple(); |
There was a problem hiding this comment.
Maybe add an extra check here, if the project_id indeed applied for the given pot? To not give the a part of the share to outside projects.
dorin-iancu
reviewed
Aug 7, 2024
contracts/potlock/src/potlock.rs
Outdated
| pub mod potlock_interactions; | ||
| pub mod potlock_storage; | ||
|
|
||
| /// An empty contract. To be used as a template when starting a new contract from scratch. |
There was a problem hiding this comment.
Don't mark as resolved if you don't fix it :nutu:
|
|
||
| #[multiversx_sc::module] | ||
| pub trait PotlockStorage { | ||
| fn is_valid_potlock_id(&self, potlock_id: PotlockId) -> bool { |
There was a problem hiding this comment.
Maybe move those "require" functions to a different file. PotlockStorage implies it only contains Storage, but there's a whole lot of other stuff here.
…or-tests Add potlock contract interactor tests
psorinionut
reviewed
Oct 14, 2024
| donation_mapper.insert(caller, payment); | ||
| } | ||
|
|
||
| // match donation_mapper.get(&caller) { |
| self.require_potlock_exists(potlock_id); | ||
| self.require_potlock_is_active(potlock_id); | ||
|
|
||
| let payment = self.call_value().single_esdt(); |
There was a problem hiding this comment.
Do you need to accept NFT payments? If not, I would change this to single_fungible_esdt.
| #[payable("*")] | ||
| #[endpoint(addPot)] | ||
| fn add_pot(&self, name: ManagedBuffer, description: ManagedBuffer) { | ||
| let payment_for_adding_pot = self.call_value().single_esdt(); |
There was a problem hiding this comment.
Not mandatory, but you could add these 2 storages under a single one, that keeps a EsdtTokenPayment struct. And you can check here if the payments are equal.
|
|
||
| #[only_admin] | ||
| #[endpoint(distributePotToProjects)] | ||
| fn distribute_pot_to_projects( |
There was a problem hiding this comment.
A few more questions here. Not mandatory but are good to be discussed.
- Right now, there is no way to be sure all the projects that applied for this pot were covered in the provided array, is it ok for the owner to be able to give the amounts arbitrary and to not all projects?
- Also, there is no check that the project applied to this specific pot, maybe add a check after you read each project from storage, that the project is active for this pot? If not, why even bother to approve a project for a pot?
- And finally, how about a limit to the number of projects that can apply for a pot? Either here for the number of elements for project_percentages, or for when subscribing a project to a pot.
| let (_, perc) = pp.into_tuple(); | ||
| total_perc += perc; | ||
| } | ||
| require!( |
There was a problem hiding this comment.
I would still enforce clear equality here.
total_perc == MAX_PERCENTAGE
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.