From 5424326aea6c93f3217b47799b54e4c640811285 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Mon, 16 Dec 2024 16:16:20 +0800 Subject: [PATCH] refactor: fix over-borrowing with meta type --- packages/account-wasm/src/account.rs | 141 ++++++++++++++++------ packages/keychain/src/utils/controller.ts | 30 +++-- 2 files changed, 126 insertions(+), 45 deletions(-) diff --git a/packages/account-wasm/src/account.rs b/packages/account-wasm/src/account.rs index 7c95830ef..84be4e9c4 100644 --- a/packages/account-wasm/src/account.rs +++ b/packages/account-wasm/src/account.rs @@ -40,6 +40,7 @@ impl CartridgeAccount { /// - `username`: Username associated with the account. /// - `signer`: A Signer struct containing the signer type and associated data. /// + #[allow(clippy::new_ret_no_self)] pub fn new( app_id: String, class_hash: JsFelt, @@ -48,7 +49,7 @@ impl CartridgeAccount { address: JsFelt, username: String, signer: Signer, - ) -> Result { + ) -> Result { set_panic_hook(); let rpc_url = Url::parse(&rpc_url)?; @@ -65,47 +66,22 @@ impl CartridgeAccount { chain_id.0, ); - Ok(CartridgeAccount { controller }) + Ok(CartridgeAccountWithMeta::new(controller)) } #[wasm_bindgen(js_name = fromStorage)] - pub fn from_storage(app_id: String) -> Result> { + pub fn from_storage(app_id: String) -> Result> { set_panic_hook(); let controller = Controller::from_storage(app_id).map_err(|e| JsError::new(&e.to_string()))?; match controller { - Some(c) => Ok(Some(CartridgeAccount { controller: c })), + Some(c) => Ok(Some(CartridgeAccountWithMeta::new(c))), None => Ok(None), } } - #[wasm_bindgen(js_name = username)] - pub fn username(&self) -> String { - self.controller.username.clone() - } - - #[wasm_bindgen(js_name = address)] - pub fn address(&self) -> String { - self.controller.address.to_hex_string() - } - - #[wasm_bindgen(js_name = classHash)] - pub fn class_hash(&self) -> String { - self.controller.class_hash.to_hex_string() - } - - #[wasm_bindgen(js_name = rpcUrl)] - pub fn rpc_url(&self) -> String { - self.controller.rpc_url.to_string() - } - - #[wasm_bindgen(js_name = chainId)] - pub fn chain_id(&self) -> String { - self.controller.chain_id.to_string() - } - #[wasm_bindgen(js_name = disconnect)] pub fn disconnect(&mut self) -> std::result::Result<(), JsControllerError> { self.controller @@ -113,11 +89,6 @@ impl CartridgeAccount { .map_err(JsControllerError::from) } - #[wasm_bindgen(js_name = ownerGuid)] - pub fn owner_guid(&self) -> JsFelt { - JsFelt(self.controller.owner_guid()) - } - #[wasm_bindgen(js_name = registerSession)] pub async fn register_session( &mut self, @@ -352,3 +323,105 @@ impl CartridgeAccount { Ok(JsFelt(res)) } } + +/// A type for accessing fixed attributes of `CartridgeAccount`. +/// +/// This type exists as concurrent mutable and immutable calls to `CartridgeAccount` are guarded +/// with `WasmMutex`, which only operates under an `async` context. If these getters were directly +/// implemented under `CartridgeAccount`: +/// +/// - calls to them would unnecessarily have to be `async` as well; +/// - there would be excessive locking. +/// +/// This type is supposed to only ever be borrowed immutably. So no concurrent access control would +/// be needed. +#[wasm_bindgen] +#[derive(Clone)] +pub struct CartridgeAccountMeta { + username: String, + address: String, + class_hash: String, + rpc_url: String, + chain_id: String, + owner_guid: JsFelt, +} + +impl CartridgeAccountMeta { + fn new(controller: &Controller) -> Self { + Self { + username: controller.username.clone(), + address: controller.address.to_hex_string(), + class_hash: controller.class_hash.to_hex_string(), + rpc_url: controller.rpc_url.to_string(), + chain_id: controller.chain_id.to_string(), + owner_guid: JsFelt(controller.owner_guid()), + } + } +} + +#[wasm_bindgen] +impl CartridgeAccountMeta { + #[wasm_bindgen(js_name = username)] + pub fn username(&self) -> String { + self.username.clone() + } + + #[wasm_bindgen(js_name = address)] + pub fn address(&self) -> String { + self.address.clone() + } + + #[wasm_bindgen(js_name = classHash)] + pub fn class_hash(&self) -> String { + self.class_hash.clone() + } + + #[wasm_bindgen(js_name = rpcUrl)] + pub fn rpc_url(&self) -> String { + self.rpc_url.clone() + } + + #[wasm_bindgen(js_name = chainId)] + pub fn chain_id(&self) -> String { + self.chain_id.clone() + } + + #[wasm_bindgen(js_name = ownerGuid)] + pub fn owner_guid(&self) -> JsFelt { + self.owner_guid.clone() + } +} + +/// A type used as the return type for constructing `CartridgeAccount` to provide an extra, +/// separately borrowable `meta` field for synchronously accessing fixed fields. +/// +/// This type exists instead of simply having `CartridgeAccount::new()` return a tuple as tuples +/// don't implement `IntoWasmAbi` which is needed for crossing JS-WASM boundary. +#[wasm_bindgen] +pub struct CartridgeAccountWithMeta { + account: CartridgeAccount, + meta: CartridgeAccountMeta, +} + +impl CartridgeAccountWithMeta { + fn new(controller: Controller) -> Self { + let meta = CartridgeAccountMeta::new(&controller); + Self { + account: CartridgeAccount { controller }, + meta, + } + } +} + +#[wasm_bindgen] +impl CartridgeAccountWithMeta { + #[wasm_bindgen(js_name = meta)] + pub fn meta(&self) -> CartridgeAccountMeta { + self.meta.clone() + } + + #[wasm_bindgen(js_name = intoAccount)] + pub fn into_account(self) -> CartridgeAccount { + self.account + } +} diff --git a/packages/keychain/src/utils/controller.ts b/packages/keychain/src/utils/controller.ts index 84cc8a373..681b258eb 100644 --- a/packages/keychain/src/utils/controller.ts +++ b/packages/keychain/src/utils/controller.ts @@ -18,6 +18,7 @@ import { toWasmPolicies } from "@cartridge/controller"; import { CartridgeAccount, + CartridgeAccountMeta, JsCall, JsFelt, JsInvocationsDetails, @@ -28,6 +29,7 @@ import { DeployedAccountTransaction } from "@starknet-io/types-js"; export default class Controller extends Account { private cartridge: CartridgeAccount; + private cartridgeMeta: CartridgeAccountMeta; constructor({ appId, @@ -50,7 +52,7 @@ export default class Controller extends Account { }) { super({ nodeUrl: rpcUrl }, address, ""); - this.cartridge = CartridgeAccount.new( + const accountWithMeta = CartridgeAccount.new( appId, classHash, rpcUrl, @@ -65,26 +67,29 @@ export default class Controller extends Account { }, }, ); + + this.cartridgeMeta = accountWithMeta.meta(); + this.cartridge = accountWithMeta.intoAccount(); } username() { - return this.cartridge.username(); + return this.cartridgeMeta.username(); } classHash() { - return this.cartridge.classHash(); + return this.cartridgeMeta.classHash(); } ownerGuid() { - return this.cartridge.ownerGuid(); + return this.cartridgeMeta.ownerGuid(); } rpcUrl() { - return this.cartridge.rpcUrl(); + return this.cartridgeMeta.rpcUrl(); } chainId() { - return this.cartridge.chainId(); + return this.cartridgeMeta.chainId(); } disconnect() { @@ -241,19 +246,22 @@ export default class Controller extends Account { } static fromStore(appId: string) { - let cartridge = CartridgeAccount.fromStorage(appId); - if (!cartridge) { + const cartridgeWithMeta = CartridgeAccount.fromStorage(appId); + if (!cartridgeWithMeta) { return; } + const meta = cartridgeWithMeta.meta(); + const controller = new Account( - { nodeUrl: cartridge.rpcUrl() }, - cartridge.address(), + { nodeUrl: meta.rpcUrl() }, + meta.address(), "", ) as Controller; Object.setPrototypeOf(controller, Controller.prototype); - controller.cartridge = cartridge; + controller.cartridge = cartridgeWithMeta.intoAccount(); + controller.cartridgeMeta = meta; return controller; } }