8000 Rename `config-helpers` to `config_helpers_sv2` and remove of unused `roles-logic-sv2` by GitGab19 · Pull Request #1812 · stratum-mining/stratum · GitHub
[go: up one dir, main page]
Skip to content

Rename config-helpers to config_helpers_sv2 and remove of unused roles-logic-sv2 - #1812

Merged
plebhash merged 3 commits into
stratum-mining:mainfrom
GitGab19:fix-config-helpers-to-publish
Jul 25, 2025
Merged

Rename config-helpers to config_helpers_sv2 and remove of unused roles-logic-sv2#1812
plebhash merged 3 commits into
stratum-mining:mainfrom
GitGab19:fix-config-helpers-to-publish

Conversation

@GitGab19
@GitGab19 GitGab19 commented Jul 24, 2025
Copy link
Copy Markdown
Member

Closes #1811

@bit-aloo
bit-aloo commented Jul 24, 2025
Copy link
Copy Markdown
Member

Pretty weird, some how unit tests are failing:

thread 'coinbase_output::tests::fixed_vector_raw' panicked at roles-utils/config-helpers/src/coinbase_output/mod.rs:300:9:
assertion `left == right` failed
  left: "Decoding hex-formatted script: odd length, failed to create bytes from hex: odd hex string length 1"
 right: "Decoding hex-formatted script: odd length, failed to create bytes from hex"
stack backtrace:

An additional suffix is getting added.

@GitGab19
GitGab19 commented Jul 24, 2025
Copy link
Copy Markdown
Member Author

I'm testing it locally and it happens only if we remove roles_logic_sv2 = { path = "../../../protocols/v2/roles-logic-sv2" } from the Cargo.toml.

But at the same time there's no real usage of roles-logic-sv2 there, that's why I was removing it.

@bit-aloo
bit-aloo commented Jul 24, 2025
Copy link
Copy Markdown
Member

I'm testing it locally and it happens only if we remove roles_logic_sv2 = { path = "../../../protocols/v2/roles-logic-sv2" } from the Cargo.toml.

But at the same time there's no real usage of roles-logic-sv2 there, that's why I was removing it.

Exactly, and there is no use of it

@plebhash
plebhash commented Jul 24, 2025 •
edited
Loading
Copy link
Copy Markdown
Member

make sure it's covered on release-libs.yaml

(just a reminder, I don't know why the test is failing)

@plebhash
plebhash commented Jul 24, 2025 •
edited
Loading
Copy link
Copy Markdown
Member

the tests were written for assertions on error messages that are generated with hex-conservative

hex-conservative arrives here as an indirect dependency

when roles_logic_sv2 is present, we get v0.3.0

when we remove roles_logic_sv2 from the dependency list, v0.2.1 from miniscript is used

but the tests were written for assertions on error messages that are generated with hex-conservative v0.3.0, and there were also some trade-offs when they were introduced: #1720 (comment)

one potential solution is to adapt the assertions for the error messages:

diff --git a/roles/roles-utils/config-helpers/src/coinbase_output/mod.rs b/roles/roles-utils/config-helpers/src/coinbase_output/mod.rs
index a4f72b9b..3a8797be 100644
--- a/roles/roles-utils/config-helpers/src/coinbase_output/mod.rs
+++ b/roles/roles-utils/config-helpers/src/coinbase_output/mod.rs
@@ -187,7 +187,7 @@ mod tests {
             CoinbaseOutput::from_descriptor("addr(1BvBMSEYstWetqTFn5Au4m4GFg7xJaNVN3)#5v55uzec")
                 .unwrap_err()
                 .to_string(),
-            "Bitcoin address: base58 error",
+            "Bitcoin address: base58 error: incorrect checksum: base58 checksum 0x6c7615f4 does not match expected 0x6b7615f4",
         );
         assert_eq!(
             CoinbaseOutput::from_descriptor(
@@ -195,14 +195,14 @@ mod tests {
             )
             .unwrap_err()
             .to_string(),
-            "Bitcoin address: base58 error",
+            "Bitcoin address: base58 error: decode: invalid base58 character 0x30",
         );
         // Flagrantly bad stuff -- should probably PR these upstream to rust-miniscript.
         assert_eq!(
             CoinbaseOutput::from_descriptor("addr()")
                 .unwrap_err()
                 .to_string(),
-            "Bitcoin address: base58 error",
+            "Bitcoin address: base58 error: too short: base58 decoded data was not long enough, must be at least 4 byte: 0",
         );
         assert_eq!(
             CoinbaseOutput::from_descriptor("addr(It's a mad mad world!?! 🙃)")
@@ -221,7 +221,7 @@ mod tests {
             CoinbaseOutput::from_descriptor("addr(It's a mad mad world!?!)#hmeprl29")
                 .unwrap_err()
                 .to_string(),
-            "Bitcoin address: base58 error",
+            "Bitcoin address: base58 error: decode: invalid base58 character 0x49",
         );
         assert_eq!(
             CoinbaseOutput::from_descriptor("addr(It's a mad mad world!?!)#🙃🙃🙃🙃🙃🙃")
@@ -293,7 +293,7 @@ mod tests {
             CoinbaseOutput::from_descriptor("raw(0)")
                 .unwrap_err()
                 .to_string(),
-            "Decoding hex-formatted script: odd length, failed to create bytes from hex",
+            "Decoding hex-formatted script: odd length, failed to create bytes from hex: odd hex string length 1",
         );
         assert_eq!(
             CoinbaseOutput::from_descriptor("raw(0,1)")

another simpler (and more future-proof) approach is to simply not try to assert for error messages

we can simply assert that an error ocurred, and adapt the error messages into comments to give context to developers in the future

so instead of doing:

assert_eq!(
    CoinbaseOutput::from_descriptor("test descriptor")
        .unwrap_err()
        .to_string(),
    "error message",
);

just do:

// context for expected error
assert!(CoinbaseOutput::from_descriptor("test descriptor").is_err())

@GitGab19
GitGab19 force-pushed the fix-config-helpers-to-publish branch 2 times, most recently from eb3b7c6 to 6799aef Compare July 25, 2025 08:35
@GitGab19
GitGab19 commented Jul 25, 2025
Copy link
Copy Markdown
Member Author

@plebhash thanks for your suggestions!

I updated those tests in 6799aef.

@GitGab19
test: make config-helpers error assertions dependency-agnostic
df346c0 
Replace brittle error message checks with general error assertions
to handle hex-conservative version differences (v0.2.1 vs v0.3.0).
@GitGab19
GitGab19 force-pushed the fix-config-helpers-to-publish branch from 6799aef to df346c0 Compare July 25, 2025 09:00
bit-aloo
bit-aloo approved these changes Jul 25, 2025
View reviewed changes
@bit-aloo bit-aloo left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. This hex stuff was pretty subtle.

@plebhash
plebhash merged commit 9949070 into stratum-mining:main Jul 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bit-aloo bit-aloo bit-aloo approved these changes
@plebhash plebhash plebhash approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

config-helpers is not published to crates.io

3 participants

@GitGab19 @bit-aloo @plebhash
0