Refactor and rename fil_token state layer (#20)

* rename fil_token -> fil_fungible_token

* fungible_token: rename some public structs

* panic in load_state if state tree missing

* use a shared-memory blockstore for testing

* streamline balance and allowance increase/decreases into single 'change_by' functions

* replace lt < 0 with is_negative

* cleanup

Author: alexytsu

Cargo.toml

@@ -3,5 +3,5 @@
 members = [
     "fvm_dispatch",
     "fvm_dispatch_tools",
-    "fil_token",
+    "fil_fungible_token",
 ]

fil_token/Cargo.toml renamed to fil_fungible_token/Cargo.toml

@@ -1,5 +1,5 @@
 [package]
-name = "fil_token"
+name = "fil_fungible_token"
 version = "0.1.0"
 edition = "2021"
 
@@ -12,6 +12,7 @@ fvm_ipld_amt = { version = "0.4.2", features = ["go-interop"] }
 fvm_ipld_encoding = "0.2.2"
 fvm_sdk = { version = "1.0.0" }
 fvm_shared = { version = "0.8.0" }
+num-traits = { version = "0.2.15" }
 serde = { version = "1.0.136", features = ["derive"] }
 serde_tuple = { version = "0.5.0" }
 thiserror = { version = "1.0.31" }

fil_token/README.md renamed to fil_fungible_token/README.md

@@ -1,17 +1,19 @@
-//! Blockstore implementation is borrowed from https://github.com/filecoin-project/builtin-actors/blob/6df845dcdf9872beb6e871205eb34dcc8f7550b5/runtime/src/runtime/actor_blockstore.rs
-//! This impl will likely be made redundant if low-level SDKs export blockstore implementations
-use std::convert::TryFrom;
-
 use anyhow::{anyhow, Result};
 use cid::multihash::Code;
 use cid::Cid;
 use fvm_ipld_blockstore::Block;
 use fvm_sdk::ipld;
+use std::cell::RefCell;
+use std::collections::HashMap;
+use std::convert::TryFrom;
+use std::rc::Rc;
 
 /// A blockstore that delegates to IPLD syscalls.
 #[derive(Default, Debug, Copy, Clone)]
 pub struct Blockstore;
 
+/// Blockstore implementation is borrowed from https://github.com/filecoin-project/builtin-actors/blob/6df845dcdf9872beb6e871205eb34dcc8f7550b5/runtime/src/runtime/actor_blockstore.rs
+/// This impl will likely be made redundant if low-level SDKs export blockstore implementations
 impl fvm_ipld_blockstore::Blockstore for Blockstore {
     fn get(&self, cid: &Cid) -> Result<Option<Vec<u8>>> {
         // If this fails, the _CID_ is invalid. I.e., we have a bug.
@@ -42,31 +44,51 @@ impl fvm_ipld_blockstore::Blockstore for Blockstore {
     }
 }
 
-// TODO: put this somewhere more appropriate when a tests folder exists
-/// An in-memory blockstore impl taken from filecoin-project/ref-fvm
+/// An in-memory blockstore impl that shares underlying memory when cloned
+///
+/// This is useful in tests to simulate a blockstore which pipes syscalls to the fvm_ipld_blockstore
 #[derive(Debug, Default, Clone)]
-pub struct MemoryBlockstore {
-    blocks: RefCell<HashMap<Cid, Vec<u8>>>,
+pub struct SharedMemoryBlockstore {
+    blocks: Rc<RefCell<HashMap<Cid, Vec<u8>>>>,
 }
 
-use std::{cell::RefCell, collections::HashMap};
-impl MemoryBlockstore {
+impl SharedMemoryBlockstore {
     pub fn new() -> Self {
         Self::default()
     }
 }
 
-impl fvm_ipld_blockstore::Blockstore for MemoryBlockstore {
+impl fvm_ipld_blockstore::Blockstore for SharedMemoryBlockstore {
     fn has(&self, k: &Cid) -> Result<bool> {
-        Ok(self.blocks.borrow().contains_key(k))
+        Ok(RefCell::borrow(&self.blocks).contains_key(k))
     }
 
     fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>> {
-        Ok(self.blocks.borrow().get(k).cloned())
+        Ok(RefCell::borrow(&self.blocks).get(k).cloned())
     }
 
     fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<()> {
-        self.blocks.borrow_mut().insert(*k, block.into());
+        RefCell::borrow_mut(&self.blocks).insert(*k, block.into());
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod test {
+    use fvm_ipld_blockstore::Blockstore;
+    use fvm_ipld_encoding::CborStore;
+    use fvm_shared::bigint::{bigint_ser::BigIntDe, BigInt};
+
+    use super::*;
+
+    #[test]
+    fn it_shares_memory_under_clone() {
+        let bs = SharedMemoryBlockstore::new();
+        let a_number = BigIntDe(BigInt::from(123));
+        let cid = bs.put_cbor(&a_number, Code::Blake2b256).unwrap();
+
+        let bs_cloned = bs.clone();
+        assert_eq!(bs.blocks, bs_cloned.blocks);
+        assert_eq!(bs.get(&cid).unwrap(), bs_cloned.get(&cid).unwrap())
+    }
+}

fil_token/src/blockstore.rs renamed to fil_fungible_token/src/blockstore.rs

@@ -2,6 +2,8 @@ pub mod errors;
 pub mod receiver;
 mod state;
 mod types;
+use std::ops::Neg;
+
 use self::state::TokenState;
 pub use self::types::*;
 
@@ -10,30 +12,33 @@ use anyhow::Ok;
 use anyhow::Result;
 use cid::Cid;
 use fvm_ipld_blockstore::Blockstore as IpldStore;
-use fvm_shared::bigint::Zero;
 use fvm_shared::econ::TokenAmount;
 use fvm_shared::ActorID;
+use num_traits::Signed;
 
 /// Library functions that implement core FRC-??? standards
 ///
 /// Holds injectable services to access/interface with IPLD/FVM layer.
-pub struct TokenHelper<BS>
+pub struct Token<BS>
 where
     BS: IpldStore + Clone,
 {
-    /// Injected blockstore
+    /// Injected blockstore. The blockstore must reference the same underlying storage under Clone
     bs: BS,
     /// Root of the token state tree
-    token_state: Cid,
+    state_cid: Cid,
 }
 
-impl<BS> TokenHelper<BS>
+impl<BS> Token<BS>
 where
     BS: IpldStore + Clone,
 {
     /// Instantiate a token helper with access to a blockstore and runtime
     pub fn new(bs: BS, token_state: Cid) -> Self {
-        Self { bs, token_state }
+        Self {
+            bs,
+            state_cid: token_state,
+        }
     }
 
     /// Constructs the token state tree and saves it at a CID
@@ -43,22 +48,27 @@ where
     }
 
     /// Helper function that loads the root of the state tree related to token-accounting
-    fn load_state(&self) -> Result<TokenState> {
-        TokenState::load(&self.bs, &self.token_state)
+    ///
+    /// Actors can't usefully recover if state wasn't initialized (failure to call `init_state`) in
+    /// the constructor so this method panics if the state tree if missing
+    fn load_state(&self) -> TokenState {
+        TokenState::load(&self.bs, &self.state_cid).unwrap()
     }
 
     /// Mints the specified value of tokens into an account
     ///
-    /// If the total supply or account balance overflows, this method returns an error. The mint
-    /// amount must be non-negative or the method returns an error.
+    /// The mint amount must be non-negative or the method returns an error
     pub fn mint(&self, initial_holder: ActorID, value: TokenAmount) -> Result<()> {
-        if value.lt(&TokenAmount::zero()) {
+        if value.is_negative() {
             bail!("value of mint was negative {}", value);
         }
 
         // Increase the balance of the actor and increase total supply
-        let mut state = self.load_state()?;
-        state.increase_balance(&self.bs, initial_holder, &value)?;
+        let mut state = self.load_state();
+        state.change_balance_by(&self.bs, initial_holder, &value)?;
+
+        // TODO: invoke the receiver hook on the initial_holder
+
         state.increase_supply(&value)?;
 
         // Commit the state atomically if supply and balance increased
@@ -72,7 +82,7 @@ where
     /// This equals the sum of `balance_of` called on all addresses. This equals sum of all
     /// successful `mint` calls minus the sum of all successful `burn`/`burn_from` calls
     pub fn total_supply(&self) -> TokenAmount {
-        let state = self.load_state().unwrap();
+        let state = self.load_state();
         state.supply
     }
 
@@ -81,7 +91,7 @@ where
     /// Accounts that have never received transfers implicitly have a zero-balance
     pub fn balance_of(&self, holder: ActorID) -> Result<TokenAmount> {
         // Load the HAMT holding balances
-        let state = self.load_state()?;
+        let state = self.load_state();
         state.get_balance(&self.bs, holder)
     }
 
@@ -90,27 +100,27 @@ where
     /// The allowance is the amount that the spender can transfer or burn out of the owner's account
     /// via the `transfer_from` and `burn_from` methods.
     pub fn allowance(&self, owner: ActorID, spender: ActorID) -> Result<TokenAmount> {
-        let state = self.load_state()?;
+        let state = self.load_state();
         let allowance = state.get_allowance_between(&self.bs, owner, spender)?;
         Ok(allowance)
     }
 
     /// Increase the allowance that a spender controls of the owner's balance by the requested delta
     ///
-    /// Returns an error if requested delta is negative or there are errors in (de)sereliazation of
+    /// Returns an error if requested delta is negative or there are errors in (de)serialization of
     /// state. Else returns the new allowance.
     pub fn increase_allowance(
         &self,
         owner: ActorID,
         spender: ActorID,
         delta: TokenAmount,
     ) -> Result<TokenAmount> {
-        if delta.lt(&TokenAmount::zero()) {
+        if delta.is_negative() {
             bail!("value of delta was negative {}", delta);
         }
 
-        let mut state = self.load_state()?;
-        let new_amount = state.increase_allowance(&self.bs, owner, spender, &delta)?;
+        let mut state = self.load_state();
+        let new_amount = state.change_allowance_by(&self.bs, owner, spender, &delta)?;
         state.save(&self.bs)?;
 
         Ok(new_amount)
@@ -119,29 +129,28 @@ where
     /// Decrease the allowance that a spender controls of the owner's balance by the requested delta
     ///
     /// If the resulting allowance would be negative, the allowance between owner and spender is set
-    /// to zero. If resulting allowance is zero, the entry is removed from the state map. Returns an
-    /// error if either the spender or owner address is unresolvable. Returns an error if requested
-    /// delta is negative. Else returns the new allowance
+    /// to zero. Returns an error if either the spender or owner address is unresolvable. Returns an
+    /// error if requested delta is negative. Else returns the new allowance
     pub fn decrease_allowance(
         &self,
         owner: ActorID,
         spender: ActorID,
         delta: TokenAmount,
     ) -> Result<TokenAmount> {
-        if delta.lt(&TokenAmount::zero()) {
+        if delta.is_negative() {
             bail!("value of delta was negative {}", delta);
         }
 
-        let mut state = self.load_state()?;
-        let new_allowance = state.decrease_allowance(&self.bs, owner, spender, &delta)?;
+        let mut state = self.load_state();
+        let new_allowance = state.change_allowance_by(&self.bs, owner, spender, &delta.neg())?;
         state.save(&self.bs)?;
 
         Ok(new_allowance)
     }
 
     /// Sets the allowance between owner and spender to 0
     pub fn revoke_allowance(&self, owner: ActorID, spender: ActorID) -> Result<()> {
-        let mut state = self.load_state()?;
+        let mut state = self.load_state();
         state.revoke_allowance(&self.bs, owner, spender)?;
         state.save(&self.bs)?;
         Ok(())
@@ -157,37 +166,37 @@ where
     /// - The target's balance MUST decrease by the requested value
     /// - The total_supply MUST decrease by the requested value
     ///
-    /// ## Operator equals target address
-    /// If the operator is the targeted address, they are implicitly approved to burn an unlimited
+    /// ## Spender equals owner address
+    /// If the spender is the targeted address, they are implicitly approved to burn an unlimited
     /// amount of tokens (up to their balance)
     ///
-    /// ## Operator burning on behalf of target address
-    /// If the operator is burning on behalf of the target token holder the following preconditions
+    /// ## Spender burning on behalf of owner address
+    /// If the spender is burning on behalf of the owner the following preconditions
     /// must be met on top of the general burn conditions:
-    /// - The operator MUST have an allowance not less than the requested value
+    /// - The spender MUST have an allowance not less than the requested value
     /// In addition to the general postconditions:
-    /// - The target-operator allowance MUST decrease by the requested value
+    /// - The target-spender allowance MUST decrease by the requested value
     ///
-    /// If the burn operation would result in a negative balance for the targeted address, the burn
-    /// is discarded and this method returns an error
+    /// If the burn operation would result in a negative balance for the owner, the burn is
+    /// discarded and this method returns an error
     pub fn burn(
         &self,
         spender: ActorID,
         owner: ActorID,
         value: TokenAmount,
     ) -> Result<TokenAmount> {
-        if value.lt(&TokenAmount::zero()) {
-            bail!("Cannot burn a negative amount");
+        if value.is_negative() {
+            bail!("cannot burn a negative amount");
         }
 
-        let mut state = self.load_state()?;
+        let mut state = self.load_state();
 
         if spender != owner {
             // attempt to use allowance and return early if not enough
             state.attempt_use_allowance(&self.bs, spender, owner, &value)?;
         }
         // attempt to burn the requested amount
-        let new_amount = state.decrease_balance(&self.bs, owner, &value)?;
+        let new_amount = state.change_balance_by(&self.bs, owner, &value.neg())?;
 
         // if both succeeded, atomically commit the transaction
         state.save(&self.bs)?;
@@ -208,42 +217,43 @@ where
     /// - The senders's balance MUST decrease by the requested value
     /// - The receiver's balance MUST increase by the requested value
     ///
-    /// ## Operator equals target address
-    /// If the operator is the 'from' address, they are implicitly approved to transfer an unlimited
+    /// ## Spender equals owner address
+    /// If the spender is the owner address, they are implicitly approved to transfer an unlimited
     /// amount of tokens (up to their balance)
     ///
-    /// ## Operator transferring on behalf of target address
-    /// If the operator is transferring on behalf of the target token holder the following preconditions
+    /// ## Spender transferring on behalf of owner address
+    /// If the spender is transferring on behalf of the target token holder the following preconditions
     /// must be met on top of the general burn conditions:
-    /// - The operator MUST have an allowance not less than the requested value
+    /// - The spender MUST have an allowance not less than the requested value
     /// In addition to the general postconditions:
-    /// - The from-operator allowance MUST decrease by the requested value
+    /// - The owner-spender allowance MUST decrease by the requested value
     pub fn transfer(
         &self,
-        operator: ActorID,
-        from: ActorID,
-        to: ActorID,
+        spender: ActorID,
+        owner: ActorID,
+        receiver: ActorID,
         value: TokenAmount,
     ) -> Result<()> {
-        if value.lt(&TokenAmount::zero()) {
-            bail!("Cannot transfer a negative amount");
+        if value.is_negative() {
+            bail!("cannot transfer a negative amount");
         }
 
-        let mut state = self.load_state()?;
+        let mut state = self.load_state();
 
-        if operator != from {
+        if spender != owner {
             // attempt to use allowance and return early if not enough
-            state.attempt_use_allowance(&self.bs, operator, from, &value)?;
+            state.attempt_use_allowance(&self.bs, spender, owner, &value)?;
         }
 
+        // attempt to credit the receiver
+        state.change_balance_by(&self.bs, receiver, &value)?;
+        // attempt to debit from the sender
+        state.change_balance_by(&self.bs, owner, &value.neg())?;
+
         // call the receiver hook
         // FIXME: use fvm_dispatch to make a standard runtime call to the receiver
         // - ensure the hook did not abort
-
-        // attempt to debit from the sender
-        state.decrease_balance(&self.bs, from, &value)?;
-        // attempt to credit the receiver
-        state.increase_balance(&self.bs, to, &value)?;
+        // - receiver hook should see the new balances...
 
         // if all succeeded, atomically commit the transaction
         state.save(&self.bs)?;