feat(ADR-006): POAP v2

[manu0466]

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...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• 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)

[giorgionocera]

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

desmos-contracts/contracts/poap/src/execute.rs

Lines 158 to 164 in 2f66294

	pub fn update_minter(
	&self,
	deps: DepsMut,
	_env: Env,
	info: MessageInfo,
	minter: String,
	) -> Result<Response<C>, ContractError> {

and UpdateMinter message

desmos-contracts/contracts/poap/src/msg.rs

Line 77 in 2f66294

UpdateMinter { minter: String },

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.

desmos-contracts/contracts/poap/src/execute.rs

Lines 44 to 49 in 2f66294

	let minter = msg
	.minter
	.map(|minter| deps.api.addr_validate(&minter))
	.transpose()?
	.unwrap_or(info.sender);
	self.minter.save(deps.storage, &minter)?;

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

Thank you for considering this proposal.

[giorgionocera]

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]

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: #190 (comment). 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]

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

1. Allow the admin to perform the same operations as the minter;
2. Allow the minter to be None;
3. 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]

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

1. Allow the admin to perform the same operations as the minter;
2. Allow the minter to be None;
3. 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!

[RiccardoM]

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).

[giorgionocera]

Thank you again for your work.
Only one thing which could improve readability (it is not a bug or an improvement, it is only a suggestion, you can also discard it):
maybe you could move the self.assert_user_dont_own_a_poap(deps.storage, &user_addr)?; instruction inside the mint_to_user and avoid using it in the mint_to and assert_user_can_mint functions.
Feel free to do your evaluations.

[manu0466]

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.

[RiccardoM]

Looks good to me overall. Let's wait a final review from @giorgionocera and merge this if it's OK

[giorgionocera]

Overall, it seems fine to me. Thank you @manu0466 and @RiccardoM! 🧑‍🚀