feat(ADR-006): POAP v2 - Githubissues

manu0466 commented 1 year ago

Description

This PR updates the POAP contract to make it compliant with the ADR-006.

Closes: #182 Closes: #183 Closes: #184 Closes: #185 Closes: #186 Closes: #187 Closes: #188 Closes: #189

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.

I have...

[x] included the correct type prefix in the PR title
[ ] added ! to the type prefix if API or client breaking change
[x] targeted the correct branch (see PR Targeting)
[ ] provided a link to the relevant issue or specification
[x] included the necessary unit and integration tests
[ ] added a changelog entry to CHANGELOG.md
[ ] updated the relevant documentation or specification
[ ] reviewed "Files changed" and left comments if necessary
[ ] confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.

I have...

[ ] confirmed the correct type prefix in the PR title
[ ] confirmed ! in the type prefix if API or client breaking change
[ ] confirmed all author checklist items have been addressed
[ ] reviewed contract logic
[ ] reviewed contract security
[ ] reviewed API design and naming
[ ] reviewed documentation is accurate
[ ] reviewed tests and test coverage
[ ] manually tested (if applicable)

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 75.11% and project coverage change: -5.17 :warning:

Comparison is base (4683373) 99.23% compared to head (93febe0) 94.06%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #195 +/- ## ========================================== - Coverage 99.23% 94.06% -5.17% ========================================== Files 15 14 -1 Lines 1046 944 -102 ========================================== - Hits 1038 888 -150 - Misses 8 56 +48 ``` | [Impacted Files](https://app.codecov.io/gh/desmos-labs/desmos-contracts/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs) | Coverage Δ | | |---|---|---| | [contracts/poap/src/lib.rs](https://app.codecov.io/gh/desmos-labs/desmos-contracts/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-Y29udHJhY3RzL3BvYXAvc3JjL2xpYi5ycw==) | `0.00% <0.00%> (ø)` | | | [contracts/remarkables/src/contract.rs](https://app.codecov.io/gh/desmos-labs/desmos-contracts/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-Y29udHJhY3RzL3JlbWFya2FibGVzL3NyYy9jb250cmFjdC5ycw==) | `99.45% <ø> (+1.06%)` | :arrow_up: | | [contracts/tips/src/contract.rs](https://app.codecov.io/gh/desmos-labs/desmos-contracts/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-Y29udHJhY3RzL3RpcHMvc3JjL2NvbnRyYWN0LnJz) | `100.00% <ø> (+0.53%)` | :arrow_up: | | [contracts/poap/src/query.rs](https://app.codecov.io/gh/desmos-labs/desmos-contracts/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-Y29udHJhY3RzL3BvYXAvc3JjL3F1ZXJ5LnJz) | `65.21% <65.21%> (ø)` | | | [contracts/poap/src/execute.rs](https://app.codecov.io/gh/desmos-labs/desmos-contracts/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-Y29udHJhY3RzL3BvYXAvc3JjL2V4ZWN1dGUucnM=) | `79.53% <79.53%> (ø)` | | | [contracts/poap/src/state.rs](https://app.codecov.io/gh/desmos-labs/desmos-contracts/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-Y29udHJhY3RzL3BvYXAvc3JjL3N0YXRlLnJz) | `100.00% <100.00%> (ø)` | | ... and [3 files with indirect coverage changes](https://app.codecov.io/gh/desmos-labs/desmos-contracts/pull/195/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

giorgionocera commented 1 year ago

Hello,

Thank you for your work on this codebase.

I would like to propose a change regarding the handling of the minter field.

In certain scenarios, it could be desirable to have the ability to set the minter to None. For instance, if one wants to ensure that only the admin can mint tokens, it might make sense to not set a third-party minter.

Currently, the update_minter function https://github.com/desmos-labs/desmos-contracts/blob/2f6629455090a1418fb5e77ffef5610c0edf50a1/contracts/poap/src/execute.rs#L158-L164 and UpdateMinter message https://github.com/desmos-labs/desmos-contracts/blob/2f6629455090a1418fb5e77ffef5610c0edf50a1/contracts/poap/src/msg.rs#L77 does not accept None as a valid value for the minter. I propose that this be changed to accept None values.

This change could include:

Modifying the update_minter function to accept None as a valid value for minter.
Updating the UpdateMinter message to allow for None as a valid minter value.

In addition, in this scenario, we might consider allowing None as a minter during the contract instantiation phase. https://github.com/desmos-labs/desmos-contracts/blob/2f6629455090a1418fb5e77ffef5610c0edf50a1/contracts/poap/src/execute.rs#L44-L49

This change could provide additional flexibility for the management of minting permissions.

Thank you for considering this proposal.

giorgionocera commented 1 year ago

I also noticed that there are currently no checks in place to ensure that the mint_start_time and mint_end_time provided are greater than the current time.

A future timestamp for both mint_start_time and mint_end_time should be used to ensure logical consistency in the token minting process. Moreover, it seems logical that a token-minting operation should occur in the future, not in the past.

I propose adding a validation step to the relevant functions to check that both mint_start_time and mint_end_time are strictly greater than the current time.

giorgionocera commented 1 year ago

While the current implementation is different, in the context of a "proof of participation" system, it would be logical for each user to hold a maximum of one token per event, indicating whether they have participated or not as explained here: https://github.com/desmos-labs/desmos-contracts/pull/190#discussion_r1260969108. If we aim for a more generic implementation where users can hold multiple tokens, we might consider implementing a poap_mint_limit_per_address variable, which specifies the maximum number of tokens that can be minted per address. What do you think about this? Thank you!

manu0466 commented 1 year ago

@RiccardoM @giorgionocera I update the contract adding the following features:

Allow the admin to perform the same operations as the minter;
Allow the minter to be None;
Limit the POAP that an user can mint to 1.

As for the checks to make sure that the mint_start_time and mint_end_time are in the future, I think they are not necessary, also not having such checks can avoid problems. For example, suppose you want to instantiate the contract with mint_start_time in the next 10 seconds, if the message is not included in the next 2 blocks the transaction will fail, because 12 seconds have elapsed from when the transaction was transmitted to when it is included.

giorgionocera commented 1 year ago

@RiccardoM @giorgionocera I update the contract adding the following features:

Allow the admin to perform the same operations as the minter;

Allow the minter to be None;

Limit the POAP that an user can mint to 1.

As for the checks to make sure that the mint_start_time and mint_end_time are in the future, I think they are not necessary, also not having such checks can avoid problems. For example, suppose you want to instantiate the contract with mint_start_time in the next 10 seconds, if the message is not included in the next 2 blocks the transaction will fail, because 12 seconds have elapsed from when the transaction was transmitted to when it is included.

Yeah, it makes sense. Thank you for your clarification and for the effort!

manu0466 commented 1 year ago

Overall looks good to me. However, it seems to me that there are some duplicated tests. Particularly:

admin_can_mint_before_start_time and admin_can_mint_after_end_time are duplicates of admin_can_mint_outside_mint_time

minter_can_mint_before_start_time and minter_can_mint_after_end_time are duplicates of minter_can_mint_outside_mint_time

user_can_mint_after_start_time and user_can_mint_before_end_time are duplicates of user_can_mint_during_mint_time

We could probably delete the single ones and keep only the composite ones (<xxx>can_mint_outside/during_mint_time).

@RiccardoM The tests are actualy different since in the xxx_can_mint_during_mint_time the contract is initialized with bot start and end time while in the xxx_can_mint_after_start_time and xxx_can_mint_before_end_time the contract is initialized with only the start time or end time.

desmos-labs / desmos-contracts

feat(ADR-006): POAP v2 #195

Description

Author Checklist

Reviewers Checklist

Codecov Report