feature: optional withdrawal address #149

taitruong · 2023-12-28T13:02:52Z

This feature adds an optional withdrawal address. Please note: existing 3rd party doesnt need to update. Like it is still possible using old instantiate msg without defining a withdraw address!

This PR allows current NFT contracts to:

set and remove withdraw addresses by owner
withdraw funds by any to given address

This way launchpads, fee sharing, marketplaces etc. can benefit from this, like:

by sending funds or royalties to nft contract where a creator on the other can withdraw them
define other 3rd party contracts to withdraw funds

Added these new messages to cw721-base:

ExecuteMsg:

    /// Sets address to send withdrawn fees to. Only owner can call this.
    SetWithdrawAddress { address: String },
    /// Removes the withdraw address, so fees are sent to the contract. Only owner can call this.
    RemoveWithdrawAddress {},
    /// Withdraw from the contract to the given address. Anyone can call this,
    /// which is okay since withdraw address has been set by owner.
    WithdrawFunds { amount: Coin },

QueryMsg:

    #[returns(String)]
    GetWithdrawAddress {},

…resses

… on latest contract.

taitruong · 2023-12-28T13:04:04Z

contracts/cw721-base/src/msg.rs

+    /// Sets address to send withdrawn fees to. Only owner can call this.
+    SetWithdrawAddress { address: String },
+    /// Removes the withdraw address, so fees are sent to the contract. Only owner can call this.
+    RemoveWithdrawAddress {},
+    /// Withdraw from the contract to the given address. Anyone can call this,
+    /// which is okay since withdraw address has been set by owner.
+    WithdrawFunds { amount: Coin },


Execute msgs for new feature here

taitruong · 2023-12-28T13:04:18Z

contracts/cw721-base/src/msg.rs

+    #[returns(Option<String>)]
+    GetWithdrawAddress {},


taitruong · 2023-12-28T13:05:05Z

contracts/cw721-base/src/state.rs

@@ -19,6 +19,7 @@ where
    /// Stored as (granter, operator) giving operator full control over granter's account
    pub operators: Map<'a, (&'a Addr, &'a Addr), Expiration>,
    pub tokens: IndexedMap<'a, &'a str, TokenInfo<T>, TokenIndexes<'a, T>>,
+    pub withdraw_address: Item<'a, String>,


new state here

taitruong · 2023-12-28T13:06:14Z

contracts/cw721-base/src/execute.rs

+    pub fn set_withdraw_address(
+        &self,
+        deps: DepsMut,
+        sender: &Addr,
+        address: String,
+    ) -> Result<Response<C>, ContractError> {
+        cw_ownable::assert_owner(deps.storage, sender)?;
+        deps.api.addr_validate(&address)?;
+        self.withdraw_address.save(deps.storage, &address)?;
+        Ok(Response::new()
+            .add_attribute("action", "set_withdraw_address")
+            .add_attribute("address", address))
+    }


Only owner (aka creator/minter) can set withdraw address

taitruong · 2023-12-28T13:06:39Z

contracts/cw721-base/src/execute.rs

+    pub fn remove_withdraw_address(
+        &self,
+        storage: &mut dyn Storage,
+        sender: &Addr,
+    ) -> Result<Response<C>, ContractError> {
+        cw_ownable::assert_owner(storage, sender)?;
+        let address = self.withdraw_address.may_load(storage)?;
+        match address {
+            Some(address) => {
+                self.withdraw_address.remove(storage);
+                Ok(Response::new()
+                    .add_attribute("action", "remove_withdraw_address")
+                    .add_attribute("address", address))
+            }
+            None => Err(ContractError::NoWithdrawAddress {}),
+        }
+    }


Only owner can remove withdraw address

taitruong · 2023-12-28T13:13:17Z

contracts/cw721-base/src/contract_tests.rs

+fn test_withdraw_funds() {
+    let mut deps = mock_dependencies();
+    let contract = setup_contract(deps.as_mut());
+
+    // no withdraw address set
+    let err = contract
+        .withdraw_funds(deps.as_mut().storage, &Coin::new(100, "uark"))
+        .unwrap_err();
+    assert_eq!(err, ContractError::NoWithdrawAddress {});
+
+    // set and withdraw by non-owner
+    contract
+        .set_withdraw_address(deps.as_mut(), &Addr::unchecked(MINTER), "foo".to_string())
+        .unwrap();
+    contract
+        .withdraw_funds(deps.as_mut().storage, &Coin::new(100, "uark"))
+        .unwrap();
+}


test withdraw_funds()

taitruong · 2023-12-28T13:15:18Z

contracts/cw721-base/src/multi_tests.rs

+/// Test backward compatibility using instantiate msg from a 0.16 version on latest contract.
+/// This ensures existing 3rd party contracts doesnt need to updated as well.
+#[test]
+fn test_instantiate_016_msg() {
+    use cw721_base_016 as v16;
+    let mut app = App::default();
+    let admin = || Addr::unchecked("admin");
+
+    let code_id_latest = app.store_code(cw721_base_latest_contract());
+
+    let cw721 = app
+        .instantiate_contract(
+            code_id_latest,
+            admin(),
+            &v16::InstantiateMsg {
+                name: "collection".to_string(),
+                symbol: "symbol".to_string(),
+                minter: admin().into_string(),
+            },
+            &[],
+            "cw721-base",
+            Some(admin().into_string()),
+        )
+        .unwrap();
+
+    // assert withdraw address is None
+    let withdraw_addr: Option<String> = app
+        .wrap()
+        .query_wasm_smart(cw721, &crate::QueryMsg::<Empty>::GetWithdrawAddress {})
+        .unwrap();
+    assert!(withdraw_addr.is_none());
+}


Test backward compatibility using instantiate msg from a 0.16 version on latest contract. This ensures existing 3rd party contracts doesnt need to be updated.
Basically NFT contracts behaves like in previous version - this PR just adds an optional feature.

taitruong · 2023-12-28T13:16:02Z

contracts/cw721-fixed-price/src/msg.rs

@@ -14,6 +14,7 @@ pub struct InstantiateMsg {
    pub cw20_address: Addr,
    pub token_uri: String,
    pub extension: Extension,
+    pub withdraw_address: Option<String>,


optional as well on fix price contract

taitruong · 2023-12-28T13:16:37Z

contracts/cw721-metadata-onchain/src/lib.rs

@@ -93,6 +93,7 @@ mod tests {
                name: "".into(),
                symbol: "".into(),
                minter: "larry".into(),
+                withdraw_address: None,


optional as well on metadata onchain contract

taitruong · 2023-12-28T13:16:57Z

contracts/cw721-non-transferable/src/lib.rs

@@ -47,6 +47,7 @@ pub mod entry {
            name: msg.name,
            symbol: msg.symbol,
            minter: msg.minter,
+            withdraw_address: msg.withdraw_address,


optional as well on non transferrable contract

JakeHartnell

I'm cool with this, but would like to make sure there is broad consensus before we move forward. Otherwise, LGTM.

taitruong · 2023-12-28T22:11:53Z

@shanev could u have a look at this? PR allows now NFT contracts having funds. Owner can set/remove withdraw address - which is used for WithdrawFunds msg.

withdraw_address is optional and backward compatible.

shanev

Just some minor comments.

shanev · 2023-12-29T20:39:27Z

Cargo.toml

-cw20            = "1.1.0"
-cw721           = { version = "0.18.0", path = "./packages/cw721" }
-cw721-base      = { version = "0.18.0", path = "./contracts/cw721-base" }
+cosmwasm-schema = "^1.2"


Version numbers should be explicit for smart contracts.

shanev · 2023-12-29T20:39:49Z

Makefile.toml

@@ -21,6 +21,7 @@ command = "cargo"
 args    = ["build", "--release", "--locked", "--target", "wasm32-unknown-unknown"]

 [tasks.optimize]
+# https://hub.docker.com/r/cosmwasm/workspace-optimizer/tags https://hub.docker.com/r/cosmwasm/workspace-optimizer-arm64/tags


Is this mean't to be commented out?

shanev · 2023-12-29T20:42:21Z

contracts/cw721-base/src/msg.rs

@@ -157,6 +167,9 @@ pub enum QueryMsg<Q: JsonSchema> {
    /// Extension query
    #[returns(())]
    Extension { msg: Q },
+
+    #[returns(Option<String>)]
+    GetWithdrawAddress {},


Prefer not having explicit getter in name per Rust convention, so just WithdrawAddress {}. Developer discretion.

taitruong added 8 commits December 12, 2023 23:47

new messages for withdrawing funds, setting and removing withdraw add…

b7f79fe

…resses

cargo make schema

847603d

clippy

7a9589b

update optimizer

1c081d8

rename

e1f6ac4

Test backward compatibility using instantiate msg from a 0.16 version…

e4f640f

… on latest contract.

add optional withdraw address

08bacaf

update lib versions

4289d23

taitruong commented Dec 28, 2023

View reviewed changes

cargo schema

8691baf

taitruong requested a review from JakeHartnell December 28, 2023 13:41

taitruong mentioned this pull request Dec 28, 2023

Incoming proxy public-awesome/cw-ics721#76

Merged

taitruong requested a review from shanev December 28, 2023 21:59

JakeHartnell approved these changes Dec 28, 2023

View reviewed changes

shanev approved these changes Dec 29, 2023

View reviewed changes

taitruong merged commit cc27e27 into CosmWasm:main Dec 29, 2023
6 checks passed

taitruong mentioned this pull request Jan 10, 2024

release 0.19.0 #150

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: optional withdrawal address #149

feature: optional withdrawal address #149

taitruong commented Dec 28, 2023

taitruong Dec 28, 2023

taitruong Dec 28, 2023

taitruong Dec 28, 2023

taitruong Dec 28, 2023

taitruong Dec 28, 2023

taitruong Dec 28, 2023

taitruong Dec 28, 2023 •

edited

taitruong Dec 28, 2023

taitruong Dec 28, 2023

taitruong Dec 28, 2023

JakeHartnell left a comment

taitruong commented Dec 28, 2023 •

edited

shanev left a comment

shanev Dec 29, 2023

shanev Dec 29, 2023

shanev Dec 29, 2023

feature: optional withdrawal address #149

feature: optional withdrawal address #149

Conversation

taitruong commented Dec 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taitruong Dec 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeHartnell left a comment

Choose a reason for hiding this comment

taitruong commented Dec 28, 2023 • edited

shanev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taitruong Dec 28, 2023 •

edited

taitruong commented Dec 28, 2023 •

edited