Code Review: Smart contract VM 01/30/19

CONTRIBUTORS.md

@@ -0,0 +1,116 @@
+# Contributing to Blockstack Core
+
+Blockstack Core is open-source software written in Rust.  Contributions
+should adhere to the following best practices.
+
+## Simplicity of implementation
+
+The most important consideration when accepting or rejecting a contribution is
+the simplicity (i.e. ease of understanding) of its implementation.
+Contributions that are "clever" or introduce functionality beyond the scope of
+the immediate problem they are meant to solve will be rejected.
+
+### Type simplicity
+
+Simplicity of implementation includes simplicity of types.  Type parameters
+and associated types should only be used if there are at
+least two possible implementations of those types.
+
+Lifetime parameters should only be introduced if the compiler cannot deduce them
+on its own.
+
+## Builds with a stable Rust compiler
+
+We use a recent, stable Rust compiler.  Contributions should _not_
+require nightly Rust features to build and run.
+
+## Each file should include relevant unit tests
+
+Each Rust file should contain a `mod test {}` definition, in which unit tests
+should be supplied for the file's methods.  Unit tests should cover a maximal
+amount of code paths.
+
+## Use built-in logging facilities
+
+Blockstack Core implements logging macros in `util::log`.  If your code needs to
+output data, it should use these macros _exclusively_ for doing so.  The only
+exception is code that is explicitly user-facing, such as help documentation.
+
+## Minimal dependencies
+
+Adding new package dependencies is very much discouraged.  Exceptions will be
+granted on a case-by-case basis, and only if deemed absolutely necessary.
+
+## Minimal global macros
+
+Adding new global macros is discouraged.  Exceptions will only be given if
+absolutely necessary.
+
+## Minimal compiler warnings
+
+Contributions should not trigger compiler warnings if possible, and should not
+mask compiler warnings with macros.  Common sources of compiler warnings that
+will not be accepted include, but are not limited to:
+
+* unnecessary imports
+* unused code
+* variable naming conventions
+* unhandled return types
+
+## Minimal `unsafe` code
+
+Contributions should not contain `unsafe` blocks if at all possible.
+
+## Code block consistency
+
+Surrounding code blocks with `{` and `}` is encouraged, even when the enclosed
+block is a single statement.  Blocks in the same lexical scope must use
+consistent conventions.  For example, consider the following:
+
+```
+match foo {
+   1..2 => {
+      // this is a single statement, but it is surrounded
+      // with { and } because the other blocks in the match
+      // statement need them.
+      Ok(true)
+   },
+   3..4 => {
+      error!("Bad value for foo");
+      Err(Error::BadFoo)
+   },
+   _ => {
+      // similarly, this block uses { }
+      Ok(true)
+   }
+}
+
+// conversely, all of the arms of this match statement
+// have one-statement blocks, so { and } can be elided.
+match bar {
+   1..2 => Some("abc"),
+   3..4 => Some("def"),
+   _ => None
+}  
+```
+
+## Error definitions
+
+Each module should include an `Error` enumeration in its `mod.rs` that encodes
+errors specific to the module.  All error code paths in the module should return
+an `Err` type with one of the module's errors.
+
+## Whitespace
+
+All contributions should use the same whitespacing as the rest of the project.
+Moreover, Pull requests where a large number of changes only deal with whitespace will be
+rejected.
+
+## Licensing and contributor license agreement.
+
+Blockstack Core is released under the terms of the GPL version 3.  Contributions
+that are not licensed under compatible terms will be rejected.  Moreover,
+contributions will not be accepted unless _all_ authors accept the project's
+contributor license agreement.
+
+

Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "blockstack-core"
 version = "0.0.1"
-authors = ["Jude Nelson <jude@blockstack.com>"]
+authors = ["Jude Nelson <jude@blockstack.com>", "Aaron Blankstein <aaron@blockstack.com>"]
 license = "GPLv3"
 homepage = "https://github.com/blockstack/blockstack-core"
 repository = "https://github.com/blockstack/blockstack-core"
@@ -13,7 +13,6 @@ readme = "README.md"
 bitcoin = "0.14.2"
 jsonrpc = "0.10.2"
 rust-ini = "0.13"
-log = "0.4"
 rand = "0.5"
 serde = "1"
 serde_derive = "1"
@@ -31,7 +30,7 @@ version = "0.16.0"
 # features = ["bundled"]
 
 [dependencies.ed25519-dalek]
-version = "1.0.0-pre.0"
+version = "=1.0.0-pre.0"
 features = ["serde"]
 
 [dependencies.curve25519-dalek]

circle.yml

@@ -3,7 +3,7 @@ jobs:
   build:
     working_directory: ~/blockstack
     docker:
-      - image: circleci/rust
+      - image: ablankstein/circleci-rust-kcov
     steps:
       - checkout
       - run:
@@ -13,5 +13,6 @@ jobs:
           environment:
             RUST_BACKTRACE: 1
             BLOCKSTACK_DEBUG: 1
+            RUSTFLAGS: -Clink-dead-code
           command: |
-            cargo test -- --nocapture --test-threads=1
+            cargo kcov && bash <(curl -s https://codecov.io/bash)

src/burnchains/bitcoin/address.rs

@@ -18,13 +18,15 @@
 */
 
 use burnchains::Address;
-use burnchains::Hash160;
-use burnchains::bitcoin::indexer::BitcoinNetworkType;
+use burnchains::bitcoin::BitcoinNetworkType;
 
 use burnchains::bitcoin::Error as btc_error;
 
 use bitcoin::util::base58;
 
+use util::hash::Hash160;
+use util::log;
+
 #[derive(Debug, PartialEq, Copy, Clone, Serialize, Deserialize)]
 pub enum BitcoinAddressType {
     PublicKeyHash,
@@ -173,16 +175,25 @@ impl Address for BitcoinAddress {
     fn to_bytes(&self) -> Vec<u8> {
         self.bytes.as_bytes().to_vec()
     }
+
+    fn to_string(&self) -> String {
+        self.to_b58()
+    }
+
+    fn from_string(s: &String) -> Option<BitcoinAddress> {
+        match BitcoinAddress::from_b58(s) {
+            Ok(a) => Some(a),
+            Err(_e) => None
+        }
+    }
 }
 
 #[cfg(test)]
 mod tests {
     use super::{BitcoinAddress, BitcoinAddressType};
-    use burnchains::bitcoin::indexer::BitcoinNetworkType;
-    use burnchains::Address;
-    use burnchains::Hash160;
-    use util::log as logger;
-    use util::hash::hex_bytes;
+    use burnchains::bitcoin::BitcoinNetworkType;
+    use util::log;
+    use util::hash::{hex_bytes, Hash160};
 
     struct AddressFixture {
         addr: String,
@@ -196,8 +207,6 @@ mod tests {
 
     #[test]
     fn test_from_b58() {
-        logger::init();
-
         let fixtures = vec![
             AddressFixture {
                 addr: "mr6nrMvvh44sR5MiX929mMXP5hqgaTr6fx".to_owned(),
@@ -263,7 +272,7 @@ mod tests {
                     test_debug!("Decoded an address when we should not have");
                     assert!(false);
                 }
-                (Err(_e), Some(res)) => {
+                (Err(_e), Some(_res)) => {
                     test_debug!("Failed to decode when we should have: {}", fixture.addr);
                     assert!(false);
                 }