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.
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
base: main
Are you sure you want to change the base?
Add Potlock contract #76
Changes from 4 commits
9832d6e
e641553
12572ae
622069f
cc7cea6
3e52a79
849325b
89ed86f
6fc6db6
4103456
214e117
a8bb80a
ae32eff
a4ecfb3
d0555ce
e35a545
d6fa92b
b479a1b
3af9363
8b4903e
2da0c7a
a5894c5
68b4660
2bbbe84
2abeab0
324f598
c01041d
dbe90fb
a1e9c73
b6bdb74
a9187f1
56fc27b
f074305
3bc8bf5
f406d80
d7c4857
906e146
c8032b0
e8cd551
75f9f47
8b51d5f
7e28fa2
eadba85
f804a7d
28addac
e416e52
8223852
c39f4d5
091289e
6c7a767
0d838e9
b7c0cd8
7e030b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Don't mark as resolved if you don't fix it :nutu:
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 few more questions here. Not mandatory but are good to be discussed.
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.
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.
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.
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.
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.
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.
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.
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?
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.
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.
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.
Only one payment token for all pots? And an ESDT? So I guess it will be WEGLD?
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.
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.
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.
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 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.
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.
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.
Add this inside the Pot struct.
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.
Moved to Pot struct.
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.
Here as well, I'd add a lastProjectId storage, and keep it independent from the projects storage size.
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.
See my reply from potlocks.
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.
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)
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 don't think this respects the specs. I don't see where an admin/owner can accept/reject an application. Docs say this:
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.
The
new
function forProject::new
will create a Project withStatus = Inactive
.An authority would have to call
endpoint(acceptApplication)
to set the project status to ActiveThere 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.
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:
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.
Accepted multiple payments but only with the same token_id.
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.
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.
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.
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.
Does not work as intended when you delete data from the potlocks storage. See the comment from the addPot endpoint.
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.
See my reply there.