Merge branch 'tomas+brentstone/common-write-storage-trait' (#331)

* tomas+brentstone/common-write-storage-trait:
  changelog: add #331
  update wasm checksums
  fix missing StorageWrite for Storage merkle tree update
  storage: remove unnecessary clone
  add more comments for StorageRead lifetime with an example usage
  add <'iter> lifetime to StorageRead trait to get around lack of GATs
  ledger: impl StorageWrite for Storage
  ledger: factor out TxEnv write methods into a new StorageWrite trait

.changelog/unreleased/improvements/331-common-write-storage-trait.md

@@ -0,0 +1,2 @@
+- Added a StorageWrite trait for a common interface for transactions and direct
+  storage access for protocol ([#331](https://github.com/anoma/namada/pull/331))

shared/src/ledger/native_vp.rs

@@ -150,7 +150,7 @@ where
     }
 }
 
-impl<'f, 'a, DB, H, CA> StorageRead for CtxPreStorageRead<'f, 'a, DB, H, CA>
+impl<'f, 'a, DB, H, CA> StorageRead<'f> for CtxPreStorageRead<'f, 'a, DB, H, CA>
 where
     DB: 'static + storage::DB + for<'iter> storage::DBIter<'iter>,
     H: 'static + StorageHasher,
@@ -210,7 +210,8 @@ where
     }
 }
 
-impl<'f, 'a, DB, H, CA> StorageRead for CtxPostStorageRead<'f, 'a, DB, H, CA>
+impl<'f, 'a, DB, H, CA> StorageRead<'f>
+    for CtxPostStorageRead<'f, 'a, DB, H, CA>
 where
     DB: 'static + storage::DB + for<'iter> storage::DBIter<'iter>,
     H: 'static + StorageHasher,

shared/src/ledger/storage/mod.rs

@@ -12,7 +12,7 @@ use tendermint::merkle::proof::Proof;
 use thiserror::Error;
 
 use super::parameters::Parameters;
-use super::storage_api::{ResultExt, StorageRead};
+use super::storage_api::{ResultExt, StorageRead, StorageWrite};
 use super::{parameters, storage_api};
 use crate::ledger::gas::MIN_STORAGE_GAS;
 use crate::ledger::parameters::EpochDuration;
@@ -424,12 +424,15 @@ where
     pub fn write(
         &mut self,
         key: &Key,
-        value: impl AsRef<[u8]> + Clone,
+        value: impl AsRef<[u8]>,
     ) -> Result<(u64, i64)> {
+        // Note that this method is the same as `StorageWrite::write_bytes`,
+        // but with gas and storage bytes len diff accounting
         tracing::debug!("storage write key {}", key,);
-        self.block.tree.update(key, value.clone())?;
+        let value = value.as_ref();
+        self.block.tree.update(key, &value)?;
 
-        let len = value.as_ref().len();
+        let len = value.len();
         let gas = key.len() + len;
         let size_diff =
             self.db.write_subspace_val(self.last_height, key, value)?;
@@ -439,6 +442,8 @@ where
     /// Delete the specified subspace and returns the gas cost and the size
     /// difference
     pub fn delete(&mut self, key: &Key) -> Result<(u64, i64)> {
+        // Note that this method is the same as `StorageWrite::delete`,
+        // but with gas and storage bytes len diff accounting
         let mut deleted_bytes_len = 0;
         if self.has_key(key)?.0 {
             self.block.tree.delete(key)?;
@@ -685,10 +690,7 @@ where
     }
 }
 
-// The `'iter` lifetime is needed for the associated type `PrefixIter`.
-// Note that the `D: DBIter<'iter>` bound uses another higher-rank lifetime
-// (see https://doc.rust-lang.org/nomicon/hrtb.html).
-impl<'iter, D, H> StorageRead for &'iter Storage<D, H>
+impl<'iter, D, H> StorageRead<'iter> for Storage<D, H>
 where
     D: DB + for<'iter_> DBIter<'iter_>,
     H: StorageHasher,
@@ -721,7 +723,7 @@ where
     }
 
     fn iter_prefix(
-        &self,
+        &'iter self,
         prefix: &crate::types::storage::Key,
     ) -> std::result::Result<Self::PrefixIter, storage_api::Error> {
         Ok(self.db.iter_prefix(prefix))
@@ -758,6 +760,53 @@ where
     }
 }
 
+impl<D, H> StorageWrite for Storage<D, H>
+where
+    D: DB + for<'iter> DBIter<'iter>,
+    H: StorageHasher,
+{
+    fn write<T: borsh::BorshSerialize>(
+        &mut self,
+        key: &crate::types::storage::Key,
+        val: T,
+    ) -> storage_api::Result<()> {
+        let val = val.try_to_vec().into_storage_result()?;
+        self.write_bytes(key, val)
+    }
+
+    fn write_bytes(
+        &mut self,
+        key: &crate::types::storage::Key,
+        val: impl AsRef<[u8]>,
+    ) -> storage_api::Result<()> {
+        // Note that this method is the same as `Storage::write`, but without
+        // gas and storage bytes len diff accounting, because it can only be
+        // used by the protocol that has a direct mutable access to storage
+        let val = val.as_ref();
+        self.block.tree.update(key, &val).into_storage_result()?;
+        let _ = self
+            .db
+            .write_subspace_val(self.block.height, key, val)
+            .into_storage_result()?;
+        Ok(())
+    }
+
+    fn delete(
+        &mut self,
+        key: &crate::types::storage::Key,
+    ) -> storage_api::Result<()> {
+        // Note that this method is the same as `Storage::delete`, but without
+        // gas and storage bytes len diff accounting, because it can only be
+        // used by the protocol that has a direct mutable access to storage
+        self.block.tree.delete(key).into_storage_result()?;
+        let _ = self
+            .db
+            .delete_subspace_val(self.block.height, key)
+            .into_storage_result()?;
+        Ok(())
+    }
+}
+
 impl From<MerkleTreeError> for Error {
     fn from(error: MerkleTreeError) -> Self {
         Self::MerkleTreeError(error)

shared/src/ledger/storage_api/mod.rs

@@ -3,13 +3,32 @@
 
 mod error;
 
-use borsh::BorshDeserialize;
+use borsh::{BorshDeserialize, BorshSerialize};
 pub use error::{CustomError, Error, Result, ResultExt};
 
 use crate::types::storage::{self, BlockHash, BlockHeight, Epoch};
 
 /// Common storage read interface
-pub trait StorageRead {
+///
+/// If you're using this trait and having compiler complaining about needing an
+/// explicit lifetime parameter, simply use trait bounds with the following
+/// syntax:
+///
+/// ```rust,ignore
+/// where
+///     S: for<'iter> StorageRead<'iter>
+/// ```
+///
+/// If you want to know why this is needed, see the to-do task below. The
+/// syntax for this relies on higher-rank lifetimes, see e.g.
+/// <https://doc.rust-lang.org/nomicon/hrtb.html>.
+///
+/// TODO: once GATs are stabilized, we should be able to remove the `'iter`
+/// lifetime param that is currently the only way to make the prefix iterator
+/// typecheck in the `<D as DBIter<'iter>>::PrefixIter` associated type used in
+/// `impl StorageRead for Storage` (shared/src/ledger/storage/mod.rs).
+/// See <https://github.com/rust-lang/rfcs/blob/master/text/1598-generic_associated_types.md>
+pub trait StorageRead<'iter> {
     /// Storage read prefix iterator
     type PrefixIter;
 
@@ -28,7 +47,13 @@ pub trait StorageRead {
 
     /// Storage prefix iterator. It will try to get an iterator from the
     /// storage.
-    fn iter_prefix(&self, prefix: &storage::Key) -> Result<Self::PrefixIter>;
+    ///
+    /// For a more user-friendly iterator API, use [`fn@iter_prefix`] or
+    /// [`fn@iter_prefix_bytes`] instead.
+    fn iter_prefix(
+        &'iter self,
+        prefix: &storage::Key,
+    ) -> Result<Self::PrefixIter>;
 
     /// Storage prefix iterator for. It will try to read from the storage.
     fn iter_next(
@@ -51,3 +76,23 @@ pub trait StorageRead {
     /// current transaction is being applied.
     fn get_block_epoch(&self) -> Result<Epoch>;
 }
+
+/// Common storage write interface
+pub trait StorageWrite {
+    /// Write a value to be encoded with Borsh at the given key to storage.
+    fn write<T: BorshSerialize>(
+        &mut self,
+        key: &storage::Key,
+        val: T,
+    ) -> Result<()>;
+
+    /// Write a value as bytes at the given key to storage.
+    fn write_bytes(
+        &mut self,
+        key: &storage::Key,
+        val: impl AsRef<[u8]>,
+    ) -> Result<()>;
+
+    /// Delete a value at the given key from storage.
+    fn delete(&mut self, key: &storage::Key) -> Result<()>;
+}

shared/src/ledger/tx_env.rs

@@ -3,34 +3,17 @@
 
 use borsh::BorshSerialize;
 
-use crate::ledger::storage_api::{self, StorageRead};
+use crate::ledger::storage_api::{StorageRead, StorageWrite};
 use crate::types::address::Address;
 use crate::types::ibc::IbcEvent;
 use crate::types::storage;
 use crate::types::time::Rfc3339String;
 
 /// Transaction host functions
-pub trait TxEnv: StorageRead {
+pub trait TxEnv<'iter>: StorageRead<'iter> + StorageWrite {
     /// Host env functions possible errors
     type Error;
 
-    /// Write a value to be encoded with Borsh at the given key to storage.
-    fn write<T: BorshSerialize>(
-        &mut self,
-        key: &storage::Key,
-        val: T,
-    ) -> Result<(), storage_api::Error>;
-
-    /// Write a value as bytes at the given key to storage.
-    fn write_bytes(
-        &mut self,
-        key: &storage::Key,
-        val: impl AsRef<[u8]>,
-    ) -> Result<(), storage_api::Error>;
-
-    /// Delete a value at the given key from storage.
-    fn delete(&mut self, key: &storage::Key) -> Result<(), storage_api::Error>;
-
     /// Write a temporary value to be encoded with Borsh at the given key to
     /// storage.
     fn write_temp<T: BorshSerialize>(

tests/src/native_vp/pos.rs

@@ -575,7 +575,6 @@ pub mod testing {
     use derivative::Derivative;
     use itertools::Either;
     use namada::ledger::pos::namada_proof_of_stake::btree_set::BTreeSetShims;
-    use namada::ledger::tx_env::TxEnv;
     use namada::types::key::common::PublicKey;
     use namada::types::key::RefTo;
     use namada::types::storage::Epoch;
@@ -590,7 +589,7 @@ pub mod testing {
     use namada_tx_prelude::proof_of_stake::{
         staking_token_address, BondId, Bonds, PosParams, Unbonds,
     };
-    use namada_tx_prelude::{Address, StorageRead};
+    use namada_tx_prelude::{Address, StorageRead, StorageWrite};
     use proptest::prelude::*;
 
     use crate::tx::{self, tx_host_env};

tests/src/vm_host_env/ibc.rs

@@ -68,6 +68,7 @@ use namada::types::ibc::data::FungibleTokenPacketData;
 use namada::types::storage::{self, BlockHash, BlockHeight};
 use namada::types::token::{self, Amount};
 use namada::vm::{wasm, WasmCacheRwAccess};
+use namada_tx_prelude::StorageWrite;
 
 use crate::tx::{self, *};
 

tests/src/vm_host_env/mod.rs

@@ -34,7 +34,9 @@ mod tests {
     use namada::types::time::DateTimeUtc;
     use namada::types::token::{self, Amount};
     use namada::types::{address, key};
-    use namada_tx_prelude::{BorshDeserialize, BorshSerialize, StorageRead};
+    use namada_tx_prelude::{
+        BorshDeserialize, BorshSerialize, StorageRead, StorageWrite,
+    };
     use namada_vp_prelude::VpEnv;
     use prost::Message;
     use test_log::test;

tx_prelude/src/ibc.rs

@@ -1,7 +1,7 @@
 //! IBC lower-level functions for transactions.
 
 pub use namada::ledger::ibc::handler::{Error, IbcActions, Result};
-use namada::ledger::storage_api::StorageRead;
+use namada::ledger::storage_api::{StorageRead, StorageWrite};
 use namada::ledger::tx_env::TxEnv;
 use namada::types::address::Address;
 pub use namada::types::ibc::IbcEvent;

tx_prelude/src/lib.rs

@@ -23,7 +23,7 @@ pub use namada::ledger::governance::storage as gov_storage;
 pub use namada::ledger::parameters::storage as parameters_storage;
 pub use namada::ledger::storage::types::encode;
 use namada::ledger::storage_api;
-pub use namada::ledger::storage_api::StorageRead;
+pub use namada::ledger::storage_api::{StorageRead, StorageWrite};
 pub use namada::ledger::treasury::storage as treasury_storage;
 pub use namada::ledger::tx_env::TxEnv;
 pub use namada::proto::{Signed, SignedTxData};
@@ -95,7 +95,7 @@ pub type TxResult = EnvResult<()>;
 #[derive(Debug)]
 pub struct KeyValIterator<T>(pub u64, pub PhantomData<T>);
 
-impl StorageRead for Ctx {
+impl StorageRead<'_> for Ctx {
     type PrefixIter = KeyValIterator<(String, Vec<u8>)>;
 
     fn read<T: BorshDeserialize>(
@@ -188,19 +188,7 @@ impl StorageRead for Ctx {
     }
 }
 
-impl TxEnv for Ctx {
-    type Error = Error;
-
-    fn get_block_time(&self) -> Result<time::Rfc3339String, Error> {
-        let read_result = unsafe { anoma_tx_get_block_time() };
-        let time_value = read_from_buffer(read_result, anoma_tx_result_buffer)
-            .expect("The block time should exist");
-        Ok(Rfc3339String(
-            String::try_from_slice(&time_value[..])
-                .expect("The conversion shouldn't fail"),
-        ))
-    }
-
+impl StorageWrite for Ctx {
     fn write<T: BorshSerialize>(
         &mut self,
         key: &namada::types::storage::Key,
@@ -227,6 +215,29 @@ impl TxEnv for Ctx {
         Ok(())
     }
 
+    fn delete(
+        &mut self,
+        key: &namada::types::storage::Key,
+    ) -> storage_api::Result<()> {
+        let key = key.to_string();
+        unsafe { anoma_tx_delete(key.as_ptr() as _, key.len() as _) };
+        Ok(())
+    }
+}
+
+impl TxEnv<'_> for Ctx {
+    type Error = Error;
+
+    fn get_block_time(&self) -> Result<time::Rfc3339String, Error> {
+        let read_result = unsafe { anoma_tx_get_block_time() };
+        let time_value = read_from_buffer(read_result, anoma_tx_result_buffer)
+            .expect("The block time should exist");
+        Ok(Rfc3339String(
+            String::try_from_slice(&time_value[..])
+                .expect("The conversion shouldn't fail"),
+        ))
+    }
+
     fn write_temp<T: BorshSerialize>(
         &mut self,
         key: &namada::types::storage::Key,
@@ -253,15 +264,6 @@ impl TxEnv for Ctx {
         Ok(())
     }
 
-    fn delete(
-        &mut self,
-        key: &namada::types::storage::Key,
-    ) -> storage_api::Result<()> {
-        let key = key.to_string();
-        unsafe { anoma_tx_delete(key.as_ptr() as _, key.len() as _) };
-        Ok(())
-    }
-
     fn insert_verifier(&mut self, addr: &Address) -> Result<(), Error> {
         let addr = addr.encode();
         unsafe { anoma_tx_insert_verifier(addr.as_ptr() as _, addr.len() as _) }

vp_prelude/src/lib.rs

@@ -295,7 +295,7 @@ impl VpEnv for Ctx {
     }
 }
 
-impl StorageRead for CtxPreStorageRead<'_> {
+impl StorageRead<'_> for CtxPreStorageRead<'_> {
     type PrefixIter = KeyValIterator<(String, Vec<u8>)>;
 
     fn read<T: BorshDeserialize>(
@@ -372,7 +372,7 @@ impl StorageRead for CtxPreStorageRead<'_> {
     }
 }
 
-impl StorageRead for CtxPostStorageRead<'_> {
+impl StorageRead<'_> for CtxPostStorageRead<'_> {
     type PrefixIter = KeyValIterator<(String, Vec<u8>)>;
 
     fn read<T: BorshDeserialize>(