Chore/Additional PoX-4 Rust Tests #4245

setzeus · 2024-01-15T23:23:32Z

Description

This PR introduces three tests for core public PoX-4 functions currently missing unit tests, specifically:

Delegate-stack-extend
Stack-increase
Delegate-stack-increase

All three tests live in .boot/pox_4_tests.rs with a few helpers defined in .boot/contract_tests.rs.

Applicable issues

N/A, general Argon/PoX-4 workflow.

Additional info (benefits, drawbacks, caveats)

Increases code coverage for PoX-4. There's a Response(DataResponse...) extract that might end up being refactored for standardization but no immediate drawbacks.

codecov · 2024-01-15T23:39:21Z

Codecov Report

Attention: 122 lines in your changes are missing coverage. Please review.

Comparison is base (9af9c0b) 24.37% compared to head (14950a2) 65.19%.
Report is 25 commits behind head on next.

Files	Patch %	Lines
...tackslib/src/chainstate/stacks/boot/pox_4_tests.rs	71.22%	82 Missing ⚠️
stackslib/src/chainstate/stacks/boot/mod.rs	61.90%	40 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             next    #4245       +/-   ##
===========================================
+ Coverage   24.37%   65.19%   +40.81%     
===========================================
  Files         432      432               
  Lines      306556   306898      +342     
===========================================
+ Hits        74735   200081   +125346     
+ Misses     231821   106817   -125004

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

friedger

👍

stackslib/src/chainstate/stacks/boot/mod.rs

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

jferrant

Looks good :D

friedger

comment related to issue #4230

friedger · 2024-01-18T16:31:29Z

stackslib/src/chainstate/stacks/boot/pox-4.clar

@@ -1144,8 +1144,8 @@
 (define-public (delegate-stack-extend
                    (stacker principal)
                    (pox-addr { version: (buff 1), hashbytes: (buff 32) })
-                    (signer-key (buff 33))
-                    (extend-count uint))
+                    (extend-count uint)


This change should be a different PR related to this issue: #4230

I already address it here & wrote tests for it accordingly. I'd rather update the description to address #4230 as well then revert the changes across all files.

That work?

The issue does not contain a preferred solution, still unclear what/whether should be implemented.

Addressed & reverted change.

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

friedger

One test needs to be fixed

friedger · 2024-01-18T16:40:36Z

stackslib/src/chainstate/stacks/boot/mod.rs

+        make_tx(key, nonce, 0, payload)
+    }
+
+    pub fn make_pox_4_delegate_increase(


This is about stacking by pool operator, not delegation. Therefore, better make_pox_4_delegate_stack_increase

friedger · 2024-01-18T16:41:54Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

+        &latest_block,
+        &key_to_stacks_addr(stacker_key).to_account_principal(),
+    )
+    .expect("No stacking state, stack-stx failed")


Why stack-stx ? Should be delegate-stack-stx

MarvinJanssen

Good to have more tests.

MarvinJanssen · 2024-01-18T21:58:45Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

+        .clone()
+        .expect_u128();
+
+    assert_eq!(total_locked, min_ustx * 2);


I would consider constructing an "expected tuple" and comparing them with assert_eq!.

For example:

stacks-core/stackslib/src/chainstate/stacks/boot/signers_tests.rs

Lines 60 to 83 in 5c8f091

assert_eq!(

readonly_call(

&mut peer,

&latest_block,

"signers".into(),

"stackerdb-get-config".into(),

vec![],

),

Value::okay(Value::Tuple(

TupleData::from_data(vec![

("chunk-size".into(), Value::UInt(4096)),

("write-freq".into(), Value::UInt(0)),

("max-writes".into(), Value::UInt(4096)),

("max-neighbors".into(), Value::UInt(32)),

(

"hint-replicas".into(),

Value::cons_list_unsanitized(vec![]).unwrap()

)

])

.unwrap()

))

.unwrap()

);

}

MarvinJanssen · 2024-01-18T21:58:57Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

+        .clone()
+        .expect_u128();
+
+    assert_eq!(total_locked, min_ustx * 2);


Same as above.

MarvinJanssen · 2024-01-18T22:01:43Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

+    assert_eq!(
+        state_signer_key.to_string(),
+        format!("0x{}", signer_public_key.to_hex())
+    );


You can compare the values directly.

friedger

Apart from #4230, this looks good.

friedger · 2024-01-19T09:43:20Z

stackslib/src/chainstate/stacks/boot/mod.rs

+        pox_addr: PoxAddress,
+        amount: u128,
+    ) -> StacksTransaction {
+        //let addr_tuple = Value::Tuple(pox_addr.as_clarity_tuple().unwrap());


Remove commented code, but add more comments :-)

friedger · 2024-01-19T09:44:06Z

stackslib/src/chainstate/stacks/boot/mod.rs

+        make_tx(key, nonce, 0, payload)
+    }
+
+    pub fn make_pox_4_stack_increase(


nit, the order of the make functions is not clear to me.

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

jcnelson · 2024-01-19T17:02:00Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

+    );
+}
+
+#[test]


It's worth adding a comment here that the reason this test is so spartan is because stack-increase is the same in pox-4 as it is in pox-3. Same goes for delegate-stack-increase.

jcnelson

Thanks for this @setzeus! Overall this looks good, but please address the other reviewers' comments. Also, could you please add docstrings to your tests, and comment on what's going on each time you perform a contract-call? Thanks!

MarvinJanssen

Some small items.

MarvinJanssen · 2024-01-19T18:11:27Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.

-use std::collections::{HashMap, HashSet, VecDeque};
+use std::collections::{BTreeMap, HashMap, HashSet, VecDeque};


Unused import?

MarvinJanssen · 2024-01-19T18:14:39Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

+    assert_eq!(
+        state_signer_key.to_string(),
+        format!("0x{}", signer_public_key.to_hex())
+    );


I think it's better to compare the values directly instead of formatting to string.

MarvinJanssen · 2024-01-19T18:14:53Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

+    assert_eq!(
+        state_signer_key_new.to_string(),
+        format!("0x{}", new_signer_public_key.to_hex())
+    );


Same as above.

kantai

These tests look good to me -- can you just add like a couple sentences of comments for each of the test in pox_4_tests that describe the test: the behavior it is trying to test, and then how it tests it?

setzeus · 2024-01-23T01:52:22Z

@kantai refactored with more reader-friendly Alice/Bob context & added comments to the three tests in this PR.

Once the comments are accepted / up to standard I'll open an issue against the remaining tests in pox_4_tests.rs to update them accordingly.

MarvinJanssen

Looks good!

MarvinJanssen · 2024-01-23T11:48:19Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

+
+    let actual_result = stacker_transactions
+        .first()
+        .map(|tx| tx.result.clone())


Is the map necessary?

Yes. Removing it breaks tests.

I mean, first() returns an Option, since you are just cloning you could perhaps use cloned().

Ah, yeap, understood & updated to use .cloned() instead.

MarvinJanssen · 2024-01-23T11:50:34Z

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs

+
+    let actual_result = delegate_transactions
+        .first()
+        .map(|tx| tx.result.clone())


Same as above.

friedger

LGTM

blockstack-devops · 2024-11-06T00:22:20Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

friedger reviewed Jan 16, 2024

View reviewed changes

jferrant self-requested a review January 18, 2024 16:08

jferrant approved these changes Jan 18, 2024

View reviewed changes

friedger reviewed Jan 18, 2024

View reviewed changes

stackslib/src/chainstate/stacks/boot/pox_4_tests.rs Outdated Show resolved Hide resolved

setzeus force-pushed the chore/additional-pox-4-rust-tests branch from 4e100b8 to def25bb Compare January 18, 2024 16:38

setzeus marked this pull request as ready for review January 18, 2024 16:39

friedger reviewed Jan 18, 2024

View reviewed changes

MarvinJanssen reviewed Jan 18, 2024

View reviewed changes

setzeus force-pushed the chore/additional-pox-4-rust-tests branch from c7ec78f to 4e814ac Compare January 19, 2024 01:40

friedger reviewed Jan 19, 2024

View reviewed changes

jcnelson reviewed Jan 19, 2024

View reviewed changes

MarvinJanssen reviewed Jan 19, 2024

View reviewed changes

setzeus force-pushed the chore/additional-pox-4-rust-tests branch from f26089c to 3d48e83 Compare January 22, 2024 18:58

setzeus requested a review from kantai January 22, 2024 20:48

kantai reviewed Jan 22, 2024

View reviewed changes

MarvinJanssen reviewed Jan 23, 2024

View reviewed changes

friedger approved these changes Jan 23, 2024

View reviewed changes

setzeus force-pushed the chore/additional-pox-4-rust-tests branch from 5d94131 to ab805d7 Compare January 23, 2024 15:31

setzeus added 8 commits January 23, 2024 12:53

added delegate-stack-extend test

4a1fbc4

added stack-increase test

f97dffc

remove TODO comment

0033991

delegate-stack-increase wip

4e6a8ce

added delegate_increase test

105f7f3

three tests complete

e93fa11

updated delegated_stack_stx_signer_key

df8d610

added make_pox_4_delegate_stack_extend to mod & cleaned up syntax

3068a68

setzeus added 17 commits January 23, 2024 12:53

refactored delegate_stack_stx_extend_signer_key

3db9401

refactored stack_increase

7a364a6

added delegate_stack_increase

2a6d71e

updated format in four assert-eqs instances

ab74174

refactored extracted values

9d036ca

last polishes

9764677

forgot formatter

46b54ef

fixed assert format

1c134f1

addressing comments

efa8cd4

removed unused import

26b23a1

removed unused import

a8b5ae9

updated comments

1f9edb1

fixed broken test

bf91827

updated asserts to check values instead of strings

82a55d0

updated with alice & bob vars & comments

2bebf82

addressing map to cloned refactor

f0c9ce0

fixed delegate-stack-extend signature

14950a2

setzeus force-pushed the chore/additional-pox-4-rust-tests branch from ab805d7 to 14950a2 Compare January 23, 2024 18:10

jferrant merged commit 3da5768 into next Jan 23, 2024
2 checks passed

blockstack-devops added the locked label Nov 6, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 6, 2024

	assert_eq!(
	readonly_call(
	&mut peer,
	&latest_block,
	"signers".into(),
	"stackerdb-get-config".into(),
	vec![],
	),
	Value::okay(Value::Tuple(
	TupleData::from_data(vec![
	("chunk-size".into(), Value::UInt(4096)),
	("write-freq".into(), Value::UInt(0)),
	("max-writes".into(), Value::UInt(4096)),
	("max-neighbors".into(), Value::UInt(32)),
	(
	"hint-replicas".into(),
	Value::cons_list_unsanitized(vec![]).unwrap()
	)
	])
	.unwrap()
	))
	.unwrap()
	);
	}

+                  );
+              }
+              #[test]

Chore/Additional PoX-4 Rust Tests #4245

Chore/Additional PoX-4 Rust Tests #4245

Conversation

setzeus commented Jan 15, 2024

Description

Applicable issues

Additional info (benefits, drawbacks, caveats)

codecov bot commented Jan 15, 2024 • edited Loading

Codecov Report

friedger left a comment

Choose a reason for hiding this comment

jferrant left a comment

Choose a reason for hiding this comment

friedger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarvinJanssen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson left a comment

Choose a reason for hiding this comment

MarvinJanssen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

setzeus commented Jan 23, 2024

MarvinJanssen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedger left a comment

Choose a reason for hiding this comment

blockstack-devops commented Nov 6, 2024

codecov bot commented Jan 15, 2024 •

edited

Loading