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

Burn to Mint #585

Closed
wants to merge 72 commits into from
Closed

Burn to Mint #585

wants to merge 72 commits into from

Conversation

humanalgorithm
Copy link
Contributor

@humanalgorithm humanalgorithm commented Jun 28, 2023

Closing in favor of #610 due to rebase errors on this branch

This PR has several modules to it:

  1. Fungible and Nonfungible msg types in sg2:
pub enum Token {
    Fungible(Coin),
    NonFungible(String),
}
  1. The branching logic to check if receiving fungible or nonfungible token in contracts. All of the use of mint price through contracts and factories had to be changed to support this. Usually this is done implicitly in the .amount() checks. For example:
let config_mint_price = config.mint_price.clone().amount()?;

The amount() function has an implicit check:

    pub fn amount(self) -> Result<Uint128, StdError> {
        let amount = match self {
            Token::Fungible(coin) => coin.amount,
            Token::NonFungible(_) => {
                return Err(StdError::generic_err("non-fungible tokens have no amount"))
            }
        };
        Ok(amount)
    }
}
  1. Refactor of open edition minter tests: The open edition minter tests were in a bad state, so I refactored them to be more consistent. The files changed are the following:
test-suite/src/common_setup/templates.rs
* in test-suite/src/open_edition_factory/
* in test-suite/src/open_edition_minter
* in test-suite/src/common_setup/setup_minter/open_edition_minter/
test-suite/src/common_setup/setup_minter/open_edition_minter.rs
  1. Missing sudo tests: there was not sufficient tests for sudo update params so I added them in the following files:
test-suite/src/base_factory/tests/sudo_tests.rs
test-suite/src/open_edition_factory/tests/sudo_tests.rs
test-suite/src/vending_factory/tests/sudo_tests.rs
  1. Updates to minters: All three minters base-minter, vending-minter and open-edition minter had the following changes:
  • Add a ReceiveNft(Cw721ReceiveMsg) path to execute
  • Add a execute_burn_to_mint function to respond to the receive NFT message
  • Updates to execute_mint functions to accomodate for not paying a mint fee when an NFT is received (collection contract will be calling itself)
  1. check_minter_caller added to s721-base: We need this assertion check because, when an NFT is received, that contract will burn that NFT and then itself call a collection for a mint. The check minter caller, allows an sg721-base collection to call itself.

  2. burn_to_mint tests: burn_to_mint.rs was added to vending-minter, open-edition-minter and base-minter tests. A send nft is sent like so:

    let send_nft = Cw721ExecuteMsg::SendNft {
        contract: minter_addr_2.to_string(),
        token_id: 1.to_string(),
        msg: to_binary("this is a test").unwrap(),
    };
    ```
    The message is sent to collection 1, with instructions to have collection 1 forward that NFT to minter 2. In this way, the functionality of burn to mint is tested end to end. 

@shanev shanev changed the base branch from main to shanev/sg-539-burn-to-mint-vending-minter July 1, 2023 19:26
@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Patch coverage: 85.03% and project coverage change: +3.83% 🎉

Comparison is base (13e288d) 59.20% compared to head (5d2c5b8) 63.04%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #585      +/-   ##
==========================================
+ Coverage   59.20%   63.04%   +3.83%     
==========================================
  Files          80       84       +4     
  Lines        4246     4662     +416     
==========================================
+ Hits         2514     2939     +425     
+ Misses       1732     1723       -9     
Files Changed Coverage Δ
contracts/collections/sg721-base/src/contract.rs 62.04% <0.00%> (+1.87%) ⬆️
...cts/minters/vending-minter-wl-flex/src/contract.rs 0.00% <0.00%> (ø)
e2e/src/helpers/helper.rs 0.00% <0.00%> (ø)
e2e/src/helpers/open_edition_minter_helpers.rs 0.00% <0.00%> (ø)
contracts/factories/base-factory/src/contract.rs 84.69% <43.75%> (+0.78%) ⬆️
...up/setup_minter/open_edition_minter/mock_params.rs 46.15% <54.54%> (-53.85%) ⬇️
packages/sg2/src/lib.rs 62.50% <62.50%> (ø)
...cts/factories/open-edition-factory/src/contract.rs 94.44% <71.42%> (+1.38%) ⬆️
...src/common_setup/setup_minter/base_minter/setup.rs 90.27% <72.00%> (-9.73%) ⬇️
...ontracts/factories/vending-factory/src/contract.rs 70.51% <83.33%> (+0.77%) ⬆️
... and 13 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@humanalgorithm humanalgorithm changed the title Burn to Mint [WIP] Burn to Mint Jul 12, 2023
@humanalgorithm humanalgorithm changed the base branch from shanev/sg-539-burn-to-mint-vending-minter to main July 12, 2023 21:40
@yubrew
Copy link
Contributor

yubrew commented Jul 26, 2023

Is this PR ready to review?

@github-actions
Copy link

github-actions bot commented Aug 5, 2023

Cosm-Orc Gas Usage

Contract Op Name Gas Used Old Gas Used Gas Diff File
vending_factory Store__Store 3202063 3044906 +5.1613% e2e/src/helpers/chain.rs:78
open_edition_factory Execute__factory_exec_minter_inst 544455 541640 +0.5197% e2e/src/tests/open_edition_minter_executes_tests.rs:59
open_edition_factory Store__Store 3708621 3434360 +7.9858% e2e/src/helpers/chain.rs:78
open_edition_factory Execute__factory_exec_minter_inst_w_trading_time 545174 542350 +0.5207% e2e/src/tests/open_edition_factory_and_mint_tests.rs:405
multiple_contracts Execute__minter_batch_exec_mint_token 18858760 3106975 +506.9814% e2e/src/tests/factory_test.rs:164
base_factory Store__Store 2811686 2665605 +5.4802% e2e/src/helpers/chain.rs:78
base_minter Store__Store 7355993 6995048 +5.1600% e2e/src/helpers/chain.rs:78
open_edition_minter Store__Store 8945659 8493792 +5.3200% e2e/src/helpers/chain.rs:78
vending_minter Store__Store 9236248 8757887 +5.4621% e2e/src/helpers/chain.rs:78
vending_minter_wl_flex Store__Store 9101016 8935500 +1.8523% e2e/src/helpers/chain.rs:78
minter Execute__minter_exec_update_per_addr_limit 210722 209170 +0.7420% e2e/src/tests/open_edition_minter_executes_tests.rs:216
Raw Report for 5d2c5b8
Contract Op Name Gas Used Gas Wanted File
vending_factory Instantiate__factory_inst 188111 259100 e2e/src/helpers/helper.rs:35
vending_factory Execute__factory_exec_minter_inst_w_trading_time 3860439 5767457 e2e/src/tests/factory_test.rs:244
vending_factory Store__Store 3202063 4779954 e2e/src/helpers/chain.rs:78
vending_factory Execute__factory_exec_minter_inst 34040236 51037134 e2e/src/tests/factory_test.rs:98
open_edition_factory Instantiate__factory_inst 187539 258192 e2e/src/helpers/open_edition_minter_helpers.rs:39
open_edition_factory Execute__factory_exec_minter_inst 544455 793563 e2e/src/tests/open_edition_minter_executes_tests.rs:59
open_edition_factory Store__Store 3708621 5539791 e2e/src/helpers/chain.rs:78
open_edition_factory Execute__factory_exec_minter_inst_w_trading_time 545174 794636 e2e/src/tests/open_edition_factory_and_mint_tests.rs:405
multiple_contracts Execute__minter_batch_exec_mint_token_w_trading_time 3113302 4647158 e2e/src/tests/open_edition_factory_and_mint_tests.rs:491
multiple_contracts Execute__minter_batch_exec_mint_token 18858760 28272284 e2e/src/tests/factory_test.rs:164
base_factory Store__Store 2811686 4194389 e2e/src/helpers/chain.rs:78
base_minter Store__Store 7355993 11010789 e2e/src/helpers/chain.rs:78
open_edition_minter Store__Store 8945659 13395288 e2e/src/helpers/chain.rs:78
sg721_base Store__Store 8384995 12554292 e2e/src/helpers/chain.rs:78
sg721_metadata_onchain Store__Store 8836004 13230806 e2e/src/helpers/chain.rs:78
sg721_nt Store__Store 7784265 11653197 e2e/src/helpers/chain.rs:78
sg721_updatable Store__Store 9161186 13718579 e2e/src/helpers/chain.rs:78
sg_whitelist Store__Store 3582040 5349920 e2e/src/helpers/chain.rs:78
sg_whitelist_flex Store__Store 3519822 5256593 e2e/src/helpers/chain.rs:78
vending_minter Store__Store 9236248 13831172 e2e/src/helpers/chain.rs:78
vending_minter_wl_flex Store__Store 9101016 13628319 e2e/src/helpers/chain.rs:78
whitelist_immutable Store__Store 2195122 3269543 e2e/src/helpers/chain.rs:78
minter Execute__minter_exec_mint_to_token 364509 523647 e2e/src/tests/open_edition_minter_executes_tests.rs:180
minter Execute__minter_exec_purge 139165 185681 e2e/src/tests/open_edition_minter_executes_tests.rs:281
minter Execute__minter_exec_update_per_addr_limit 210722 292967 e2e/src/tests/open_edition_minter_executes_tests.rs:216
minter Execute__minter_exec_update_trading_time 271526 384171 e2e/src/tests/open_edition_minter_executes_tests.rs:99

@shanev shanev requested review from yubrew, shanev and tasiov August 8, 2023 21:48
@MightOfOaks
Copy link
Member

The creators will probably want to select which collection(s) the token(s) should be burned from in order for a token to be minted from their collection. I think we need to update the MinterCreateMsgs to optionally receive this information and check which collection the Cw721ReceiveMsg originates from in the execute_burn_to_mint functions.

Also not sure if we should update mint_price to be optional as well to allow for Burn-to-Mint-only collections.

@MightOfOaks
Copy link
Member

Using this functionality with the 1/1 collections (base factory/minter) may not be ideal because the tokens are minted to the collection owner's wallet anyway. The current mint price is 5 STARS/token. Not sure if there is a use case for that.

@humanalgorithm
Copy link
Contributor Author

Using this functionality with the 1/1 collections (base factory/minter) may not be ideal because the tokens are minted to the collection owner's wallet anyway. The current mint price is 5 STARS/token. Not sure if there is a use case for that.

As discussed in Discord, we will leave the code in the base minter, not because it is likely to be used but because it will be easier to refactor in the future if the code is consistent across minters.

@humanalgorithm
Copy link
Contributor Author

The creators will probably want to select which collection(s) the token(s) should be burned from in order for a token to be minted from their collection. I think we need to update the MinterCreateMsgs to optionally receive this information and check which collection the Cw721ReceiveMsg originates from in the execute_burn_to_mint functions.

Also not sure if we should update mint_price to be optional as well to allow for Burn-to-Mint-only collections.

As discussed in Discord:

  • We will need an optional parameter that gives the list of valid collection addresses for burn to mint, rather than to just receive NFTs from random collections for payment
  • The above should be added to the CreateMinterMsg as optional parameter
  • Mint price will remain for now, with a future PR to add this. Reasoning being that it would make the PR too complicated to add this.

@@ -568,6 +611,62 @@ pub fn execute_mint_for(
)
}

fn _pay_mint_if_not_contract(
this_contract: Addr,
Copy link
Member

Choose a reason for hiding this comment

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

Also seems like duplicate code that's in other contracts.

contracts/factories/base-factory/Cargo.toml Show resolved Hide resolved
contracts/collections/sg721-base/src/contract.rs Outdated Show resolved Hide resolved
contracts/collections/sg721-base/src/contract.rs Outdated Show resolved Hide resolved
contracts/collections/sg721-base/src/contract.rs Outdated Show resolved Hide resolved
contracts/minters/base-minter/src/contract.rs Outdated Show resolved Hide resolved
contracts/minters/base-minter/src/contract.rs Outdated Show resolved Hide resolved
contracts/minters/base-minter/src/contract.rs Outdated Show resolved Hide resolved
// Create mint msgs
let mint_msg = Sg721ExecuteMsg::<Extension, Empty>::Mint {
token_id: increment_token_index(deps.storage)?.to_string(),
owner: info.sender.to_string(),
owner: collection_info.creator,
Copy link
Contributor

Choose a reason for hiding this comment

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

the NFT owner would be the collection creator?

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 NFT owner would be the collection creator?

So now that the contract can call itself through the execute_burn_to_mint messages, we can't just assume the the sender is always creator, and therefore info.sender won't cut it.

Actually looking at the name of this function execute_mint_sender I'm not entirely sure what is the intention of this function. It sounds like a mint is being performed on behalf of the sender? But here I am just putting back the collection creator because that's all I have access to to figure out who should be the owner. Unless I do a query for owner here? Thoughts?

contracts/minters/open-edition-minter/Cargo.toml Outdated Show resolved Hide resolved
contracts/minters/open-edition-minter/src/contract.rs Outdated Show resolved Hide resolved
@@ -57,8 +73,7 @@ fn check_custom_create_minter_denom() {
.ok();

setup_block_time(&mut router, GENESIS_MINT_START_TIME + 100, None);

// Mint succeeds
// // Mint succeeds
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// // Mint succeeds
// Mint succeeds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants