Remove controller.

The controller was designed to support atomic token transfer and
action on a remote chain. It has two rules:

1. If a note has a controller, only the controller may execute
   messages.
2. The controller may specify an arbitrary sender when executing
   messages.

The result of this is:

1. Users have two accounts: their outpost account, and their regular
   Polytone account.
2. The security level of the outpost account is the security level of
   the outpost, as the outpost is the controller.

While this works, it's not the greatest. Having two accounts isn't
perfect UX, and handing off account security to another contract
"feels" bad. Oak Security also classified this as a "major" issue in
their audit report, saying that the requirement that Outposts are
given unrestricted access to user accounts was quite sub-optimal.

The solution to this problem is non-obvious and complex. In their
audit report, Oak suggests requiring the outpost user to pre-authorize
a message for execution by the Outpost later. This doesn't work
because not all of the information needed to create the second message
is guarenteed to be avaliable at the time the first message is to be
executed. For example, a transfer message can't be pre-crafted for a
NFT collection being transfered to a remote chain for the first time,
as the address of the NFT collection smart contract does not yet
exist.

To resolve this, one could design some system wherein a transfer is
pre-authorized for _any_ non-existant denomination, or perhaps there
are better schemes. Unfourtunately, schemes of this type belongs in
the outpost code, not Polytone, as it requires custom code per
token-type.

What a strange loop. Starting at from the permise of how to remove the
controller, we arrive at the conclusion that we need the controller.

So why remove it? I am unsatisfied with this conclusion, the
controller is a blemish on an otherwise simple and beautiful codebase,
and off-chain it has become clear that we won't be building an outpost
with Polytone in the near future.

contracts/note/src/contract.rs

@@ -10,7 +10,7 @@ use polytone::{callback, ibc};
 use crate::error::ContractError;
 
 use crate::msg::{ExecuteMsg, InstantiateMsg, MigrateMsg, Pair, QueryMsg};
-use crate::state::{BLOCK_MAX_GAS, CHANNEL, CONNECTION_REMOTE_PORT, CONTROLLER};
+use crate::state::{BLOCK_MAX_GAS, CHANNEL, CONNECTION_REMOTE_PORT};
 
 const CONTRACT_NAME: &str = "crates.io:polytone-note";
 const CONTRACT_VERSION: &str = env!("CARGO_PKG_VERSION");
@@ -41,11 +41,6 @@ pub fn instantiate(
         CONNECTION_REMOTE_PORT.save(deps.storage, &(connection_id, remote_port))?;
     };
 
-    if let Some(controller) = msg.controller {
-        response = response.add_attribute("controller", controller.to_string());
-        CONTROLLER.save(deps.storage, &deps.api.addr_validate(&controller)?)?;
-    }
-
     Ok(response)
 }
 
@@ -56,18 +51,16 @@ pub fn execute(
     info: MessageInfo,
     msg: ExecuteMsg,
 ) -> Result<Response, ContractError> {
-    let (msg, callback, timeout_seconds, request_type, on_behalf_of) = match msg {
+    let (msg, callback, timeout_seconds, request_type) = match msg {
         ExecuteMsg::Execute {
             msgs,
             callback,
             timeout_seconds,
-            on_behalf_of,
         } => (
             ibc::Msg::Execute { msgs },
             callback,
             timeout_seconds,
             CallbackRequestType::Execute,
-            on_behalf_of,
         ),
         ExecuteMsg::Query {
             msgs,
@@ -78,37 +71,13 @@ pub fn execute(
             Some(callback),
             timeout_seconds,
             CallbackRequestType::Query,
-            None,
         ),
     };
 
-    let sender = match CONTROLLER.may_load(deps.storage)? {
-        Some(controller) => {
-            if controller != info.sender {
-                return Err(ContractError::NotController);
-            }
-
-            if request_type == CallbackRequestType::Query {
-                info.sender
-            } else if let Some(sender) = on_behalf_of {
-                deps.api.addr_validate(&sender)?
-            } else {
-                return Err(ContractError::OnBehalfOfNotSet);
-            }
-        }
-        None => {
-            if on_behalf_of.is_some() {
-                return Err(ContractError::NotControlledButOnBehalfIsSet);
-            } else {
-                info.sender
-            }
-        }
-    };
-
     callback::request_callback(
         deps.storage,
         deps.api,
-        sender.clone(),
+        info.sender.clone(),
         callback,
         request_type,
     )?;
@@ -122,7 +91,7 @@ pub fn execute(
         .add_message(IbcMsg::SendPacket {
             channel_id,
             data: to_binary(&ibc::Packet {
-                sender: sender.into_string(),
+                sender: info.sender.into_string(),
                 msg,
             })
             .expect("msgs are known to be serializable"),
@@ -140,7 +109,6 @@ pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> StdResult<Binary> {
                 remote_port,
             },
         )),
-        QueryMsg::Controller => to_binary(&CONTROLLER.may_load(deps.storage)?),
         QueryMsg::RemoteAddress { local_address } => to_binary(
             &callback::LOCAL_TO_REMOTE_ACCOUNT
                 .may_load(deps.storage, &deps.api.addr_validate(&local_address)?)?,

contracts/note/src/error.rs

@@ -19,11 +19,4 @@ pub enum ContractError {
 
     #[error("contract has no pair, establish a channel with a voice module to create one")]
     NoPair,
-
-    #[error("Note is not controlled, but 'on_behalf_of' is set")]
-    NotControlledButOnBehalfIsSet,
-    #[error("Note is controlled, but this address is not the controller")]
-    NotController,
-    #[error("Note is controlled, but 'on_behalf_of' is not set")]
-    OnBehalfOfNotSet,
 }

contracts/note/src/msg.rs

@@ -12,27 +12,6 @@ pub struct InstantiateMsg {
     /// ever be one voice for every note.
     pub pair: Option<Pair>,
 
-    /// This is the controller of the note. If a controller is set:
-    ///
-    /// 1. Only the controller may execute messages.
-    /// 2. The controller mat execute messages on behalf of any
-    ///    address.
-    ///
-    /// The controller allows sequencing of IBC-transfers and actions
-    /// on the counterparty chain. For example, the controller of a
-    /// note could:
-    ///
-    /// 1. Receive tokens from initiator.
-    /// 2. Transfer tokens to initators remote account.
-    /// 3. Perform an action on the initiator's behalf on the remote
-    ///    chain.
-    ///
-    /// For more discussion of the controller, see "How Polytone
-    /// Supports Outposts":
-    ///
-    /// <https://github.com/DA0-DA0/polytone/wiki/How-Polytone-Supports-Outposts>
-    pub controller: Option<String>,
-
     /// The max gas allowed in a transaction. When returning callbacks
     /// the module will use this to calculate the amount of gas to
     /// save for handling a callback error. This protects from
@@ -58,14 +37,16 @@ pub enum ExecuteMsg {
     /// their callbacks by calling `set_data` on their `Response`
     /// object. Optionaly, returns a callback of `Vec<Callback>` where
     /// index `i` corresponds to the callback for `msgs[i]`.
+    ///
+    /// Accounts are created on the voice chain after the first call
+    /// to execute by the local address. To create an account, but
+    /// perform no additional actions, pass an empty list to
+    /// `msgs`. Accounts are queryable via the `RemoteAddress {
+    /// local_address }` query after they have been created.
     Execute {
         msgs: Vec<CosmosMsg<Empty>>,
         callback: Option<CallbackRequest>,
         timeout_seconds: Uint64,
-
-        /// Must be `None` if no controller is set, or `Some(address)`
-        /// if a controller is set.
-        on_behalf_of: Option<String>,
     },
 }
 
@@ -79,9 +60,6 @@ pub enum QueryMsg {
     /// The contract's corresponding voice on a remote chain.
     #[returns(Option<Pair>)]
     Pair,
-    /// This note's controller, or None.
-    #[returns(Option<String>)]
-    Controller,
     /// Returns the remote address for the provided local address. If
     /// no account exists, returns `None`. An account can be created
     /// by calling `ExecuteMsg::Execute` with the sender being

contracts/note/src/state.rs

@@ -1,4 +1,3 @@
-use cosmwasm_std::Addr;
 use cw_storage_plus::Item;
 
 /// (Connection-ID, Remote port) of this contract's pair.
@@ -8,9 +7,5 @@ pub const CONNECTION_REMOTE_PORT: Item<(String, String)> = Item::new("a");
 /// no channel is active.
 pub const CHANNEL: Item<String> = Item::new("b");
 
-/// The controller of this note. See documentation for the
-/// `controller` field on `InstantiateMsg` for more information.
-pub const CONTROLLER: Item<Addr> = Item::new("c");
-
 /// Max gas usable in a single block.
 pub(crate) const BLOCK_MAX_GAS: Item<u64> = Item::new("bmg");

contracts/note/src/suite_tests/mod.rs

@@ -1,2 +1,2 @@
 mod suite;
-mod tests;
+mod tests;

contracts/note/src/suite_tests/suite.rs

@@ -33,7 +33,6 @@ impl Default for SuiteBuilder {
             instantiate: InstantiateMsg {
                 block_max_gas: Uint64::zero(),
                 pair: None,
-                controller: None,
             },
         }
     }

contracts/note/src/tests.rs

@@ -20,10 +20,7 @@ fn simple_note() {
         deps.as_mut(),
         env.clone(),
         info.clone(),
-        InstantiateMsg {
-            pair: None,
-            controller: None,
-        },
+        InstantiateMsg { pair: None },
     )
     .unwrap();
     CHANNEL
@@ -48,85 +45,3 @@ fn simple_note() {
     )
     .unwrap();
 }
-
-#[test]
-fn controlled_note() {
-    let mut deps = mock_dependencies();
-    let env = mock_env();
-    let info = mock_info("sender", &[]);
-    let controller_info = mock_info("controller", &[]);
-
-    instantiate(
-        deps.as_mut(),
-        env.clone(),
-        info.clone(),
-        InstantiateMsg {
-            pair: None,
-            controller: Some("controller".to_string()),
-        },
-    )
-    .unwrap();
-    CHANNEL
-        .save(deps.as_mut().storage, &"some_channel".to_string())
-        .unwrap();
-
-    execute(
-        deps.as_mut(),
-        env.clone(),
-        controller_info.clone(),
-        crate::msg::ExecuteMsg::Execute {
-            on_behalf_of: Some("some_addr".to_string()),
-            msgs: vec![WasmMsg::Execute {
-                contract_addr: "some_addr".to_string(),
-                msg: to_binary("some_msg").unwrap(),
-                funds: vec![],
-            }
-            .into()],
-            callback: None,
-            timeout_seconds: Uint64::new(10000),
-        },
-    )
-    .unwrap();
-
-    // note is controlled but `on_behalf_of` is not set, should error
-    let err = execute(
-        deps.as_mut(),
-        env.clone(),
-        controller_info,
-        crate::msg::ExecuteMsg::Execute {
-            on_behalf_of: None,
-            msgs: vec![WasmMsg::Execute {
-                contract_addr: "some_addr".to_string(),
-                msg: to_binary("some_msg").unwrap(),
-                funds: vec![],
-            }
-            .into()],
-            callback: None,
-            timeout_seconds: Uint64::new(10000),
-        },
-    )
-    .unwrap_err();
-
-    assert_eq!(err, ContractError::OnBehalfOfNotSet);
-
-    // note is controlled but not controller called execute, should error
-    let err = execute(
-        deps.as_mut(),
-        env,
-        info,
-        crate::msg::ExecuteMsg::Execute {
-            on_behalf_of: Some("some_addr".to_string()),
-            msgs: vec![WasmMsg::Execute {
-                contract_addr: "some_addr".to_string(),
-                msg: to_binary("some_msg").unwrap(),
-                funds: vec![],
-            }
-            .into()],
-            callback: None,
-            timeout_seconds: Uint64::new(10000),
-        },
-    )
-    .unwrap_err();
-
-    assert_eq!(err, ContractError::NotController)
-}

contracts/voice/src/lib.rs

@@ -5,4 +5,4 @@ pub mod msg;
 pub mod state;
 
 #[cfg(test)]
-mod suite_tests;
+mod suite_tests;

contracts/voice/src/suite_tests/suite.rs

@@ -1,5 +1,5 @@
-use cw_multi_test::{App, ContractWrapper, Contract, Executor, AppResponse};
 use cosmwasm_std::{Addr, Empty, Uint64};
+use cw_multi_test::{App, AppResponse, Contract, ContractWrapper, Executor};
 
 use crate::msg::QueryMsg::{BlockMaxGas, ProxyCodeId};
 use crate::msg::{InstantiateMsg, MigrateMsg};
@@ -33,7 +33,7 @@ impl Default for SuiteBuilder {
             instantiate: InstantiateMsg {
                 proxy_code_id: Uint64::zero(),
                 block_max_gas: Uint64::zero(),
-            }
+            },
         }
     }
 }
@@ -44,15 +44,16 @@ impl SuiteBuilder {
 
         let voice_code = app.store_code(voice_contract());
 
-        let voice_address = app.instantiate_contract(
-            voice_code, 
-            Addr::unchecked(CREATOR_ADDR), 
-            &self.instantiate, 
-            &[], 
-            "voice contract", 
-            Some(CREATOR_ADDR.to_string()),
-        )
-        .unwrap();
+        let voice_address = app
+            .instantiate_contract(
+                voice_code,
+                Addr::unchecked(CREATOR_ADDR),
+                &self.instantiate,
+                &[],
+                "voice contract",
+                Some(CREATOR_ADDR.to_string()),
+            )
+            .unwrap();
 
         Suite {
             app,
@@ -71,7 +72,7 @@ impl SuiteBuilder {
 impl Suite {
     pub fn store_voice_contract(&mut self) -> u64 {
         self.app.store_code(voice_contract())
-    } 
+    }
 }
 
 // query
@@ -99,16 +100,15 @@ impl Suite {
         contract_id: u64,
         block_max_gas: u64,
     ) -> anyhow::Result<AppResponse> {
-        self.app
-            .migrate_contract(
-                sender, 
-                self.voice_address.clone(), 
-                &MigrateMsg::WithUpdate { 
-                    proxy_code_id: contract_id.into(),
-                    block_max_gas: block_max_gas.into(), 
-                }, 
-                self.voice_code,
-            )
+        self.app.migrate_contract(
+            sender,
+            self.voice_address.clone(),
+            &MigrateMsg::WithUpdate {
+                proxy_code_id: contract_id.into(),
+                block_max_gas: block_max_gas.into(),
+            },
+            self.voice_code,
+        )
     }
 }
 
@@ -123,4 +123,4 @@ impl Suite {
         let curr = self.query_proxy_code_id();
         assert_eq!(curr, val);
     }
-}
+}