Merge pull request #5739 from stacks-network/chore/address-refactor

Refactor: StacksAddress and PrincipalData cleanup

.github/workflows/bitcoin-tests.yml

@@ -167,6 +167,7 @@ jobs:
           - tests::nakamoto_integrations::sip029_coinbase_change
           - tests::nakamoto_integrations::clarity_cost_spend_down
           - tests::nakamoto_integrations::v3_blockbyheight_api_endpoint
+          - tests::nakamoto_integrations::mine_invalid_principal_from_consensus_buff
           - tests::nakamoto_integrations::test_tenure_extend_from_flashblocks
           # TODO: enable these once v1 signer is supported by a new nakamoto epoch
           # - tests::signer::v1::dkg

CHANGELOG.md

@@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to the versioning scheme outlined in the [README.md](README.md).
 
-## [Unreleased]
+## [3.1.0.0.4]
 
 ### Added
 

clarity/src/libclarity.rs

@@ -60,7 +60,8 @@ pub mod boot_util {
     pub fn boot_code_id(name: &str, mainnet: bool) -> QualifiedContractIdentifier {
         let addr = boot_code_addr(mainnet);
         QualifiedContractIdentifier::new(
-            addr.into(),
+            addr.try_into()
+                .expect("FATAL: boot contract addr is not a legal principal"),
             ContractName::try_from(name.to_string())
                 .expect("FATAL: boot contract name is not a legal ContractName"),
         )

clarity/src/vm/contexts.rs

@@ -2134,20 +2134,14 @@ mod test {
         mut tl_env_factory: TopLevelMemoryEnvironmentGenerator,
     ) {
         let mut env = tl_env_factory.get_env(epoch);
-        let u1 = StacksAddress {
-            version: 0,
-            bytes: Hash160([1; 20]),
-        };
-        let u2 = StacksAddress {
-            version: 0,
-            bytes: Hash160([2; 20]),
-        };
+        let u1 = StacksAddress::new(0, Hash160([1; 20])).unwrap();
+        let u2 = StacksAddress::new(0, Hash160([2; 20])).unwrap();
         // insufficient balance must be a non-includable transaction. it must error here,
         //  not simply rollback the tx and squelch the error as includable.
         let e = env
             .stx_transfer(
-                &PrincipalData::from(u1),
-                &PrincipalData::from(u2),
+                &PrincipalData::try_from(u1).unwrap(),
+                &PrincipalData::try_from(u2).unwrap(),
                 1000,
                 &BuffData::empty(),
             )

clarity/src/vm/functions/conversions.rs

@@ -20,6 +20,7 @@ use crate::vm::errors::{
     check_argument_count, CheckErrors, InterpreterError, InterpreterResult as Result,
 };
 use crate::vm::representations::SymbolicExpression;
+use crate::vm::types::serialization::SerializationError;
 use crate::vm::types::SequenceSubtype::BufferType;
 use crate::vm::types::TypeSignature::SequenceType;
 use crate::vm::types::{
@@ -276,6 +277,9 @@ pub fn from_consensus_buff(
         env.epoch().value_sanitizing(),
     ) {
         Ok(value) => value,
+        Err(SerializationError::UnexpectedSerialization) => {
+            return Err(CheckErrors::Expects("UnexpectedSerialization".into()).into())
+        }
         Err(_) => return Ok(Value::none()),
     };
     if !type_arg.admits(env.epoch(), &result)? {

clarity/src/vm/functions/crypto.rs

@@ -27,10 +27,7 @@ use crate::vm::errors::{
     check_argument_count, CheckErrors, InterpreterError, InterpreterResult as Result,
 };
 use crate::vm::representations::SymbolicExpression;
-use crate::vm::types::{
-    BuffData, SequenceData, StacksAddressExtensions, TypeSignature, Value, BUFF_32, BUFF_33,
-    BUFF_65,
-};
+use crate::vm::types::{BuffData, SequenceData, TypeSignature, Value, BUFF_32, BUFF_33, BUFF_65};
 use crate::vm::{eval, ClarityVersion, Environment, LocalContext};
 
 macro_rules! native_hash_func {
@@ -120,7 +117,7 @@ pub fn special_principal_of(
         } else {
             pubkey_to_address_v1(pub_key)?
         };
-        let principal = addr.to_account_principal();
+        let principal = addr.into();
         Ok(Value::okay(Value::Principal(principal))
             .map_err(|_| InterpreterError::Expect("Failed to construct ok".into()))?)
     } else {

clarity/src/vm/functions/principals.rs

@@ -58,15 +58,10 @@ pub fn special_is_standard(
     runtime_cost(ClarityCostFunction::IsStandard, env, 0)?;
     let owner = eval(&args[0], env, context)?;
 
-    let version = match owner {
-        Value::Principal(PrincipalData::Standard(StandardPrincipalData(version, _bytes))) => {
-            version
-        }
-        Value::Principal(PrincipalData::Contract(QualifiedContractIdentifier {
-            issuer,
-            name: _,
-        })) => issuer.0,
-        _ => return Err(CheckErrors::TypeValueError(TypeSignature::PrincipalType, owner).into()),
+    let version = if let Value::Principal(ref p) = owner {
+        p.version()
+    } else {
+        return Err(CheckErrors::TypeValueError(TypeSignature::PrincipalType, owner).into());
     };
 
     Ok(Value::Bool(version_matches_current_network(
@@ -161,10 +156,12 @@ pub fn special_principal_destruct(
     let principal = eval(&args[0], env, context)?;
 
     let (version_byte, hash_bytes, name_opt) = match principal {
-        Value::Principal(PrincipalData::Standard(StandardPrincipalData(version, bytes))) => {
+        Value::Principal(PrincipalData::Standard(p)) => {
+            let (version, bytes) = p.destruct();
             (version, bytes, None)
         }
         Value::Principal(PrincipalData::Contract(QualifiedContractIdentifier { issuer, name })) => {
+            let issuer = issuer.destruct();
             (issuer.0, issuer.1, Some(name))
         }
         _ => {
@@ -254,7 +251,7 @@ pub fn special_principal_construct(
     // Construct the principal.
     let mut transfer_buffer = [0u8; 20];
     transfer_buffer.copy_from_slice(verified_hash_bytes);
-    let principal_data = StandardPrincipalData(version_byte, transfer_buffer);
+    let principal_data = StandardPrincipalData::new(version_byte, transfer_buffer)?;
 
     let principal = if let Some(name) = name_opt {
         // requested a contract principal.  Verify that the `name` is a valid ContractName.

clarity/src/vm/test_util/mod.rs

@@ -108,7 +108,7 @@ impl From<&StacksPrivateKey> for StandardPrincipalData {
             &vec![StacksPublicKey::from_private(o)],
         )
         .unwrap();
-        StandardPrincipalData::from(stacks_addr)
+        StandardPrincipalData::try_from(stacks_addr).unwrap()
     }
 }
 

clarity/src/vm/tests/principals.rs

@@ -668,7 +668,7 @@ fn test_principal_construct_good() {
         Value::Response(ResponseData {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Standard(
-                StandardPrincipalData(22, transfer_buffer)
+                StandardPrincipalData::new(22, transfer_buffer).unwrap()
             )))
         }),
         execute_with_parameters(
@@ -688,7 +688,7 @@ fn test_principal_construct_good() {
         Value::Response(ResponseData {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Standard(
-                StandardPrincipalData(20, transfer_buffer)
+                StandardPrincipalData::new(20, transfer_buffer).unwrap()
             )))
         }),
         execute_with_parameters(
@@ -710,7 +710,7 @@ fn test_principal_construct_good() {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Contract(
                 QualifiedContractIdentifier::new(
-                    StandardPrincipalData(22, transfer_buffer),
+                    StandardPrincipalData::new(22, transfer_buffer).unwrap(),
                     "hello-world".into()
                 )
             )))
@@ -734,7 +734,7 @@ fn test_principal_construct_good() {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Contract(
                 QualifiedContractIdentifier::new(
-                    StandardPrincipalData(20, transfer_buffer),
+                    StandardPrincipalData::new(20, transfer_buffer).unwrap(),
                     "hello-world".into()
                 )
             )))
@@ -756,7 +756,7 @@ fn test_principal_construct_good() {
         Value::Response(ResponseData {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Standard(
-                StandardPrincipalData(26, transfer_buffer)
+                StandardPrincipalData::new(26, transfer_buffer).unwrap()
             )))
         }),
         execute_with_parameters(
@@ -776,7 +776,7 @@ fn test_principal_construct_good() {
         Value::Response(ResponseData {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Standard(
-                StandardPrincipalData(21, transfer_buffer)
+                StandardPrincipalData::new(21, transfer_buffer).unwrap()
             )))
         }),
         execute_with_parameters(
@@ -798,7 +798,7 @@ fn test_principal_construct_good() {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Contract(
                 QualifiedContractIdentifier::new(
-                    StandardPrincipalData(26, transfer_buffer),
+                    StandardPrincipalData::new(26, transfer_buffer).unwrap(),
                     "hello-world".into()
                 )
             )))
@@ -822,7 +822,7 @@ fn test_principal_construct_good() {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Contract(
                 QualifiedContractIdentifier::new(
-                    StandardPrincipalData(21, transfer_buffer),
+                    StandardPrincipalData::new(21, transfer_buffer).unwrap(),
                     "hello-world".into()
                 )
             )))
@@ -853,15 +853,14 @@ fn create_principal_from_strings(
     if let Some(name) = name {
         // contract principal requested
         Value::Principal(PrincipalData::Contract(QualifiedContractIdentifier::new(
-            StandardPrincipalData(version_array[0], principal_array),
+            StandardPrincipalData::new(version_array[0], principal_array).unwrap(),
             name.into(),
         )))
     } else {
         // standard principal requested
-        Value::Principal(PrincipalData::Standard(StandardPrincipalData(
-            version_array[0],
-            principal_array,
-        )))
+        Value::Principal(PrincipalData::Standard(
+            StandardPrincipalData::new(version_array[0], principal_array).unwrap(),
+        ))
     }
 }
 

clarity/src/vm/tests/simple_apply_eval.rs

@@ -430,7 +430,7 @@ fn test_secp256k1() {
     )
     .unwrap();
     eprintln!("addr from privk {:?}", &addr);
-    let principal = addr.to_account_principal();
+    let principal = addr.try_into().unwrap();
     if let PrincipalData::Standard(data) = principal {
         eprintln!("test_secp256k1 principal {:?}", data.to_address());
     }
@@ -446,7 +446,7 @@ fn test_secp256k1() {
     )
     .unwrap();
     eprintln!("addr from hex {:?}", addr);
-    let principal = addr.to_account_principal();
+    let principal: PrincipalData = addr.try_into().unwrap();
     if let PrincipalData::Standard(data) = principal.clone() {
         eprintln!("test_secp256k1 principal {:?}", data.to_address());
     }
@@ -491,8 +491,9 @@ fn test_principal_of_fix() {
         .unwrap()],
     )
     .unwrap()
-    .to_account_principal();
-    let testnet_principal = StacksAddress::from_public_keys(
+    .try_into()
+    .unwrap();
+    let testnet_principal: PrincipalData = StacksAddress::from_public_keys(
         C32_ADDRESS_VERSION_TESTNET_SINGLESIG,
         &AddressHashMode::SerializeP2PKH,
         1,
@@ -502,7 +503,8 @@ fn test_principal_of_fix() {
         .unwrap()],
     )
     .unwrap()
-    .to_account_principal();
+    .try_into()
+    .unwrap();
 
     // Clarity2, mainnet, should have a mainnet principal.
     assert_eq!(