chore(sdk + twap-monitor): hex helpers via alloy_primitives::hex::encode#51
Closed
brunota20 wants to merge 1 commit into
Closed
chore(sdk + twap-monitor): hex helpers via alloy_primitives::hex::encode#51brunota20 wants to merge 1 commit into
brunota20 wants to merge 1 commit into
Conversation
mfw78 review of PR #8 (nullislabs#8) flagged "we already pull alloy, so pulling hex via there is really not much of a deal". The PR #47 (COW-1074) commit acc9654 then introduced two new custom hex helpers that recreate the same antipattern at a different scope: - `crates/shepherd-sdk/src/cow/app_data.rs::encode_hex` - 32-byte hash → `0x...`. Used by `resolve_app_data` to format the orderbook lookup path. - `modules/twap-monitor/src/strategy.rs::hex_short` - 8-byte prefix → `0x...…`. Used to format `appData` hashes in INFO log lines. Both crates already depend on `alloy-primitives` (sdk: 1.6, twap-monitor: 1.5), so the swap is a one-liner per call site: - `encode_hex(b)` → `format!("0x{}", alloy_primitives::hex::encode(b))` - `hex_short(b)` → `format!("0x{}…", alloy_primitives::hex::encode(&b[..8]))` Both functions keep their old signature so callers (`resolve_app_data` in the SDK, every `host.log` line in twap-monitor strategy) need no changes. Comments on both helpers now explicitly reference mfw78's PR #8 guidance so the next person tempted to hand-roll a `0123456789abcdef` table has a hook. Validation: cargo test -p shepherd-sdk -p twap-monitor: 32 + 23 passed; cargo clippy --all-targets -- -D warnings: clean; cargo fmt --check: clean; zero em-dash drift. Why this PR sits in a separate branch rather than amending PR #47: PR #47 is already In Review, and #48/#49/#50 stack on top of it. Amending would require force-pushing 4 branches. A small follow-up PR keeps each one bisectable and lets mfw78 review the alloy alignment in isolation. AI assistance disclosure: drafted by Claude (Opus 4.7, 1M context) while sweeping all open PRs for the same antipatterns mfw78 flagged on PRs #8 and #9.
Author
|
Work landed via |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mfw78's review of PR #8 flagged: "we already pull alloy, so pulling hex via there is really not much of a deal". PR #47 (COW-1074, commit acc9654) then introduced two new custom hex helpers that recreate the same antipattern at a different scope.
This PR removes both:
crates/shepherd-sdk/src/cow/app_data.rsencode_hex(&[u8; 32])0123456789abcdeftableformat!(\"0x{}\", alloy_primitives::hex::encode(b))modules/twap-monitor/src/strategy.rshex_short(&[u8; 32])format!(\"0x{}…\", alloy_primitives::hex::encode(&b[..8]))Both crates already depend on
alloy-primitives(sdk 1.6, twap-monitor 1.5), so this is a pure simplification - no Cargo.toml change. Both helpers keep their signatures so call sites (resolve_app_datain the SDK, everyhost.logline in twap-monitor strategy) need no changes. Comments on both helpers now explicitly reference mfw78's PR #8 guidance so the next person tempted to hand-roll the table has a hook.Stack
Stacked on
fix/twap-calldata-helper-cow-1077(PR #50) →feat/ethflow-expected-excessive-validto-cow-1076(PR #49) →feat/forward-orderbook-error-cow-1075(PR #48) →feat/resolve-app-data-cow-1074(PR #47).Why a separate PR rather than amending #47
PR #47 is already In Review and #48/#49/#50 stack on top. Amending #47 would force-push 4 branches and obscure the reviewer's history. A small follow-up PR keeps each commit bisectable and lets the alloy alignment be reviewed in isolation.
The other surfaces flagged by mfw78 on PRs #8 and #9 are also being addressed:
hex_encodeincrates/nexum-engine/src/host/error.rs- same antipattern in the engine binary. Fixed onfeat/supervisor-event-loop(PR feat(ethflow-watcher): decode CoWSwapEthFlow OrderPlacement (BLEU-832) #9) headdb860c0. The M4 stack will inherit it on rebase post-merge.ProviderPool::emptycfg gating - same story; tightened to#[cfg(test)]on PR feat(ethflow-watcher): decode CoWSwapEthFlow OrderPlacement (BLEU-832) #9 head.ParseError+CapabilityViolationthiserror - already corrected on PR feat(ethflow-watcher): decode CoWSwapEthFlow OrderPlacement (BLEU-832) #9 head4051979.extract_hostURL parsing viaurl::Url- already corrected on PR feat(ethflow-watcher): decode CoWSwapEthFlow OrderPlacement (BLEU-832) #9 head4051979.4051979.Validation
cargo test -p shepherd-sdk -p twap-monitor: 32 + 23 passed, 0 failed.cargo clippy --all-targets --tests -- -D warnings: clean.cargo fmt --check: clean.AI assistance disclosure: drafted by Claude (Opus 4.7, 1M context) while sweeping all open PRs for the same antipatterns mfw78 flagged on PRs #8 and #9.