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

Add Potlock contract #76

Open
wants to merge 53 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
9832d6e
Add Potlock contract
CostinCarabas Jun 17, 2024
e641553
Refactor donations
CostinCarabas Jun 18, 2024
12572ae
Fix building issues
CostinCarabas Jun 18, 2024
622069f
Merge branch 'fix-unit-tests' into add-potlock-contract
CostinCarabas Jun 20, 2024
cc7cea6
Framework upgrade potlock
CostinCarabas Jun 20, 2024
3e52a79
Add blackbox tests
CostinCarabas Jun 20, 2024
849325b
Added more unit tests
CostinCarabas Jun 21, 2024
89ed86f
Add several more tests
CostinCarabas Jun 21, 2024
6fc6db6
Add interactor
CostinCarabas Jun 21, 2024
4103456
Adjust interactors functions
CostinCarabas Jun 22, 2024
214e117
Add diferent types of actors from different wallets
CostinCarabas Jun 26, 2024
a8bb80a
Merge branch 'main' into add-potlock-contract
CostinCarabas Jul 3, 2024
ae32eff
Merge pull request #84 from multiversx/potlock-framework-upgrade
CostinCarabas Jul 3, 2024
a4ecfb3
Merge branch 'main' into add-potlock-contract
CostinCarabas Jul 11, 2024
d0555ce
Merge remote-tracking branch 'origin/main' into add-potlock-contract
CostinCarabas Jul 16, 2024
e35a545
Merge pull request #85 from multiversx/potlock-unit-tests
CostinCarabas Jul 16, 2024
d6fa92b
Merge branch 'add-potlock-contract' into potlock-system-tests
CostinCarabas Jul 16, 2024
b479a1b
Merge pull request #86 from multiversx/potlock-system-tests
CostinCarabas Jul 16, 2024
3af9363
Framework upgrade 0.51.1
CostinCarabas Jul 19, 2024
8b4903e
Fix tests
CostinCarabas Jul 19, 2024
2da0c7a
Fix clippy
CostinCarabas Jul 19, 2024
a5894c5
Fixes after review
CostinCarabas Jul 19, 2024
68b4660
Fixes after review
CostinCarabas Jul 31, 2024
2bbbe84
Fix tests
CostinCarabas Jul 31, 2024
2abeab0
Refuse multiple donations with different tokens
CostinCarabas Jul 31, 2024
324f598
Add tests for multiple donations with different tokens
CostinCarabas Jul 31, 2024
c01041d
Fix donate_to_project
CostinCarabas Jul 31, 2024
dbe90fb
Merge branch 'main' into add-potlock-contract
CostinCarabas Aug 1, 2024
a1e9c73
Framework upgrade 0.52.1
CostinCarabas Aug 1, 2024
b6bdb74
potlock: distributePotToProjects: percentage req
CostinCarabas Aug 5, 2024
a9187f1
Merge branch 'main' into add-potlock-contract
CostinCarabas Aug 6, 2024
56fc27b
Merge branch 'main' into add-potlock-contract
dorin-iancu Aug 7, 2024
f074305
Merge branch 'main' into add-potlock-contract
CostinCarabas Aug 12, 2024
3bc8bf5
potlock: Framework upgrade
CostinCarabas Aug 14, 2024
f406d80
Fix README
CostinCarabas Aug 16, 2024
d7c4857
Fixes after review
CostinCarabas Aug 19, 2024
906e146
Fixes after review
CostinCarabas Aug 19, 2024
c8032b0
Fixes after review
CostinCarabas Aug 19, 2024
e8cd551
Fix tests
CostinCarabas Aug 20, 2024
75f9f47
Fix compiling issues
CostinCarabas Aug 21, 2024
8b51d5f
potlock: more unit tests
CostinCarabas Aug 21, 2024
7e28fa2
Add interactor tests
vladiouz Aug 21, 2024
eadba85
Add interactor tests
vladiouz Sep 6, 2024
f804a7d
Solve warnings
vladiouz Sep 6, 2024
28addac
Execute interactor tests only on demand
vladiouz Sep 6, 2024
e416e52
Cleanse tests
vladiouz Sep 6, 2024
8223852
Cleanse tests
vladiouz Sep 6, 2024
c39f4d5
Merge pull request #105 from multiversx/add-potlock-contract-interact…
vladiouz Sep 6, 2024
091289e
Merge branch 'main' into add-potlock-contract
CostinCarabas Oct 7, 2024
6c7a767
Framework upgrade 0.53.2
CostinCarabas Oct 9, 2024
0d838e9
potlock: clippy fix
CostinCarabas Oct 9, 2024
b7c0cd8
Merge branch 'main' into add-potlock-contract
CostinCarabas Oct 9, 2024
7e030b5
Fixes after review
CostinCarabas Oct 24, 2024
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
300 changes: 248 additions & 52 deletions Cargo.lock

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ members = [
"contracts/paymaster/meta",
"contracts/ping-pong-egld",
"contracts/ping-pong-egld/meta",
"contracts/potlock",
"contracts/potlock/meta",
"contracts/proxy-deployer",
"contracts/proxy-deployer/meta",
"contracts/proxy-pause",
Expand Down
22 changes: 22 additions & 0 deletions contracts/potlock/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
[package]
name = "potlock"
version = "0.0.0"
authors = ["you"]
edition = "2021"
publish = false
readme = "README.md"

[lib]
path = "src/potlock.rs"

[dependencies.multiversx-sc]
version = "0.47.2"

[dependencies.multiversx-sc-modules]
version = "=0.47.2"

[dev-dependencies]
num-bigint = "0.4.2"

[dev-dependencies.multiversx-sc-scenario]
version = "0.47.2"
7 changes: 7 additions & 0 deletions contracts/potlock/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Paymaster SC

## Overview



## Implementation
12 changes: 12 additions & 0 deletions contracts/potlock/meta/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[package]
name = "potlock-meta"
version = "0.0.0"
edition = "2021"
publish = false

[dependencies.potlock]
path = ".."

[dependencies.multiversx-sc-meta]
version = "0.47.2"
default-features = false
3 changes: 3 additions & 0 deletions contracts/potlock/meta/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
fn main() {
multiversx_sc_meta::cli_main::<potlock::AbiProvider>();
}
3 changes: 3 additions & 0 deletions contracts/potlock/multiversx.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"language": "rust"
}
39 changes: 39 additions & 0 deletions contracts/potlock/scenarios/potlock.scen.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"name": "empty",
"steps": [
{
"step": "setState",
"accounts": {
"address:owner": {
"nonce": "1",
"balance": "0"
}
},
"newAddresses": [
{
"creatorAddress": "address:owner",
"creatorNonce": "1",
"newAddress": "sc:empty"
}
]
},
{
"step": "scDeploy",
"id": "deploy",
"tx": {
"from": "address:owner",
"contractCode": "mxsc:../output/potlock.mxsc.json",
"arguments": [],
"gasLimit": "5,000,000",
"gasPrice": "0"
},
"expect": {
"out": [],
"status": "",
"logs": [],
"gas": "*",
"refund": "*"
}
}
]
}
28 changes: 28 additions & 0 deletions contracts/potlock/src/potlock.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#![no_std]

multiversx_sc::imports!();
multiversx_sc::derive_imports!();
pub mod potlock_admin_interactions;
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
pub mod potlock_interactions;
pub mod potlock_setup;
pub mod potlock_storage;

/// An empty contract. To be used as a template when starting a new contract from scratch.
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't mark as resolved if you don't fix it :nutu:

#[multiversx_sc::contract]
pub trait Potlock:
potlock_admin_interactions::PotlockAdminInteractions
+ potlock_interactions::PotlockInteractions
+ potlock_setup::PotlockSetup
+ potlock_storage::PotlockStorage
+ multiversx_sc_modules::only_admin::OnlyAdminModule
{
#[init]
fn init(&self, admin: ManagedAddress) {
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
let caller = self.blockchain().get_caller();
self.admins().insert(caller);
self.admins().insert(admin);
}

#[upgrade]
fn upgrade(&self) {}
}
95 changes: 95 additions & 0 deletions contracts/potlock/src/potlock_admin_interactions.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
use crate::{
potlock_setup,
potlock_storage::{self, PotlockId, ProjectId},
};

multiversx_sc::imports!();
multiversx_sc::derive_imports!();

pub type ProjectPercentage = MultiValue2<usize, u64>;

#[multiversx_sc::module]
pub trait PotlockAdminInteractions:
potlock_storage::PotlockStorage
+ multiversx_sc_modules::only_admin::OnlyAdminModule
+ potlock_setup::PotlockSetup
{
#[only_admin]
#[endpoint(acceptPot)]
fn accept_pot(&self, potlock_id: PotlockId) {
self.require_potlock_exists(potlock_id);
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
let fee_amount = self.fee_amount().get();

self.fee_amount_accepted_pots()
.update(|amount| *amount += fee_amount);
self.fee_pot_proposer(potlock_id).clear();
}

#[only_admin]
#[endpoint(removePot)]
fn remove_pot(&self, potlock_id: PotlockId) {
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
let pot_proposer = self.fee_pot_proposer(potlock_id).get();
let fee_pot_payment = EsdtTokenPayment::new(
self.fee_token_identifier().get(),
0u64,
self.fee_amount().get(),
);

self.send()
.direct_non_zero_esdt_payment(&pot_proposer, &fee_pot_payment);
self.fee_pot_proposer(potlock_id).clear();
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
self.potlocks().clear_entry(potlock_id);
}

#[only_admin]
#[endpoint(acceptApplication)]
fn accept_application(&self, project: ProjectId) {
self.require_project_exists(project);
// TODO: Mark project's status as accepted
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
}

#[only_admin]
#[endpoint(rejectDonation)]
fn reject_donation(&self, potlock_id: PotlockId, user: ManagedAddress) {
self.require_potlock_exists(potlock_id);
let opt_fee_pot_payments = self.pot_donations(potlock_id).get(&user);
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved

require!(opt_fee_pot_payments.is_some(), "No donation for this user");
let fee_pot_payments = unsafe { opt_fee_pot_payments.unwrap_unchecked() };

self.send()
.direct_non_zero_esdt_payment(&user, &fee_pot_payments);
self.pot_donations(potlock_id).remove(&user);
}

#[only_admin]
#[endpoint(distributePotToProjects)]
fn distribute_pot_to_projects(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

&self,
potlock_id: PotlockId,
project_percentage: MultiValueEncoded<ProjectPercentage>,
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
) {
self.require_potlock_exists(potlock_id);
let pot_donations = self.pot_donations(potlock_id);

for pp in project_percentage {
let (project_id, percentage) = pp.into_tuple();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

let mut output_payments = ManagedVec::new();
for (_, donation) in pot_donations.iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are unable to iterated through all payments and distribute them if we have singleValueMapper.

let project_share_amount = donation.amount * percentage;
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
let project_share = EsdtTokenPayment::new(
donation.token_identifier,
donation.token_nonce,
project_share_amount,
);
output_payments.push(project_share);
}
let project_owner = self.projects().get(project_id).owner;
self.send().direct_multi(&project_owner, &output_payments);
}

self.pot_donations(potlock_id).clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

Because you completely clear (instead of deduct each payment), the require_correct_percentages should check that the total percentage == 100%, instead of <= 100%.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?


//TODO: Clear all info regarding the pot?
}
}
64 changes: 64 additions & 0 deletions contracts/potlock/src/potlock_interactions.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
use crate::potlock_setup;
use crate::potlock_storage::{self, Pot, Project};
use crate::potlock_storage::{PotlockId, ProjectId};

multiversx_sc::imports!();
multiversx_sc::derive_imports!();

#[multiversx_sc::module]
pub trait PotlockInteractions:
potlock_storage::PotlockStorage
+ potlock_setup::PotlockSetup
+ multiversx_sc_modules::only_admin::OnlyAdminModule
{
#[payable("*")]
#[endpoint(addPot)]
fn add_pot(&self, name: ManagedBuffer, description: ManagedBuffer) {
let payment_for_adding_pot = self.call_value().single_esdt();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

require!(
self.fee_token_identifier().get() == payment_for_adding_pot.token_identifier,
Copy link
Contributor

Choose a reason for hiding this comment

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

Only one payment token for all pots? And an ESDT? So I guess it will be WEGLD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The donations can be in any ESDT token.

"Wrong token identifier for creating a pot!"
);
require!(
self.fee_amount().get() == payment_for_adding_pot.amount,
"Wrong fee amount for creating a pot"
);
let caller = self.blockchain().get_caller();

let potlock_id = self.potlocks().len() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 potlock = Pot::new(potlock_id, name, description);
self.potlocks().push(&potlock);

self.fee_pot_proposer(potlock_id).set(caller);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this inside the Pot struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to Pot struct.

}

#[endpoint(applyForPot)]
fn apply_for_pot(
&self,
potlock_id: PotlockId,
project_name: ManagedBuffer,
description: ManagedBuffer,
) {
let project_id = self.projects().len() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well, I'd add a lastProjectId storage, and keep it independent from the projects storage size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my reply from potlocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

As previously suggested, I don't think there's a need for saving the id inside the struct. And if you remove it, you can remove the len + 1 computation, and just return the usize from the last line
self.projects().push(&project)

let owner = self.blockchain().get_caller();
let project = Project::new(project_id, potlock_id, project_name, description, owner);
self.projects().push(&project);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

#[payable("*")]
#[endpoint(donateToPot)]
fn donate_to_pot(&self, potlock_id: PotlockId) {
let payment = self.call_value().single_esdt();
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
let caller = self.blockchain().get_caller();
self.pot_donations(potlock_id).insert(caller, payment);
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. You take the output of the insert function and send back to the user
  2. You add a check if there is already a donation on behalf of that user.
  3. Instead of a EsdtTokenPayment per user, you save a PaymentsVec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepted multiple payments but only with the same token_id.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now I see you did this update for the donateToProject endpoint, but it is needed here also.

}

#[payable("*")]
#[endpoint(donateToProject)]
fn donate_to_project(&self, project_id: ProjectId) {
self.require_project_exists(project_id);
let payment = self.call_value().single_esdt();
let caller = self.blockchain().get_caller();
self.project_donations(project_id).insert(caller, payment);
}
}
42 changes: 42 additions & 0 deletions contracts/potlock/src/potlock_setup.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
use crate::potlock_storage::{self, PotlockId, ProjectId};

multiversx_sc::imports!();

#[multiversx_sc::module]
pub trait PotlockSetup:
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
potlock_storage::PotlockStorage + multiversx_sc_modules::only_admin::OnlyAdminModule
{
#[only_admin]
#[endpoint(changeFeeForPots)]
fn change_fee_for_pots(&self, token_identifier: TokenIdentifier, fee: BigUint) {
require!(
token_identifier.is_valid_esdt_identifier(),
"Invalid token provided"
);
self.fee_token_identifier().set(&token_identifier);
self.fee_amount().set(fee);
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved
}

//// internal functions
fn is_valid_potlock_id(&self, potlock_id: PotlockId) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not work as intended when you delete data from the potlocks storage. See the comment from the addPot endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my reply there.

potlock_id >= 1 && potlock_id <= self.potlocks().len()
}

fn require_potlock_exists(&self, potlock_id: PotlockId) {
require!(
self.is_valid_potlock_id(potlock_id) && !self.potlocks().item_is_empty(potlock_id),
"Potlock doesn't exist!",
)
}

fn is_valid_project_id(&self, project_id: ProjectId) -> bool {
project_id >= 1 && project_id <= self.projects().len()
}

fn require_project_exists(&self, project_id: ProjectId) {
require!(
self.is_valid_project_id(project_id) && !self.projects().item_is_empty(project_id),
"Project doesn't exist!",
)
}
}
Loading
Loading