Skip to content

chore(core): impl Eq, Hash for ArkadeAddress#199

Merged
bonomat merged 1 commit intomasterfrom
feat/Hash-eq-ark-address
Mar 31, 2026
Merged

chore(core): impl Eq, Hash for ArkadeAddress#199
bonomat merged 1 commit intomasterfrom
feat/Hash-eq-ark-address

Conversation

@bonomat
Copy link
Copy Markdown
Collaborator

@bonomat bonomat commented Mar 30, 2026

useful for storing in hashsets, hashmaps, etc

Summary by CodeRabbit

  • Chores
    • Enhanced internal address type capabilities to support additional use cases and improve robustness with extended test coverage.

useful for storing in hashsets, hashmaps, etc
@bonomat bonomat requested a review from luckysori March 30, 2026 04:22
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

Walkthrough

The ArkAddress struct now derives Eq and Hash traits in addition to existing derives, enabling use as a hashable key. A unit test verifies equality and HashSet deduplication behavior.

Changes

Cohort / File(s) Summary
Derive traits and test
ark-core/src/ark_address.rs
Added Eq and Hash derives to ArkAddress struct to enable use in hash collections. New hash_and_eq unit test verifies equality semantics and HashSet deduplication for identical and distinct addresses.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title mentions 'ArkadeAddress' but the actual struct being modified is 'ArkAddress' - this is a naming mismatch. Correct the title to 'chore(core): impl Eq, Hash for ArkAddress' to accurately reflect the struct name being modified.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/Hash-eq-ark-address

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@arkanaai
Copy link
Copy Markdown

arkanaai bot commented Mar 30, 2026

🔍 Arkana PR Review — arkade-os/rust-sdk#199

chore(core): impl Eq, Hash for ArkadeAddress

Summary

Adds Eq and Hash derives to ArkAddress, enabling use in HashSet, HashMap, and other collections that require these traits.

Review

✅ Correctness

  • ArkAddress already derived PartialEq and Copy. Adding Eq is correct since all fields (u8, Hrp, XOnlyPublicKey, TweakedPublicKey) implement Eq.
  • Hash derivation is safe here — all constituent types implement Hash, and the auto-derived impl will hash all fields, which is consistent with the existing PartialEq (satisfying the Hash/Eq contract).

✅ Test coverage

  • Test verifies both equality and hash-based deduplication in HashSet, plus distinctness for different addresses. Good coverage for a derive change.

✅ Security

  • No key handling changes, no transaction logic affected. Pure trait derivation.
  • Note: the Hash impl is deterministic within a single process run but uses Rust's default SipHash with random seeding per process, so hash values are not stable across runs. This is fine for in-memory collections but would not be suitable for persistent storage keying (not a concern here, just noting).

No issues found. Clean, minimal change. LGTM. 👍

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ark-core/src/ark_address.rs (1)

10-16: ⚠️ Potential issue | 🔴 Critical

Remove Hash and Eq from the derive list on line 10.

The Hrp type from bech32 0.11.0 does not implement Hash or Eq traits. Attempting to derive these traits on ArkAddress will fail at compile time. While TweakedPublicKey and XOnlyPublicKey support these traits, Hrp does not, blocking the entire derive operation.

Consider either removing Hash, Eq from the derive macro or wrapping Hrp in a newtype that implements the required traits.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ark-core/src/ark_address.rs` around lines 10 - 16, The derive list on
ArkAddress includes Hash and Eq but Hrp (used as the hrp field) does not
implement Hash or Eq; remove Hash and Eq from the derive macro on ArkAddress
(keep Debug, Clone, Copy, PartialEq) or alternatively wrap Hrp in a newtype that
implements Hash and Eq and use that newtype as the hrp field; update references
to ArkAddress or the hrp field accordingly and ensure TweakedPublicKey and
XOnlyPublicKey remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@ark-core/src/ark_address.rs`:
- Around line 10-16: The derive list on ArkAddress includes Hash and Eq but Hrp
(used as the hrp field) does not implement Hash or Eq; remove Hash and Eq from
the derive macro on ArkAddress (keep Debug, Clone, Copy, PartialEq) or
alternatively wrap Hrp in a newtype that implements Hash and Eq and use that
newtype as the hrp field; update references to ArkAddress or the hrp field
accordingly and ensure TweakedPublicKey and XOnlyPublicKey remain unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 04f98254-862b-4528-abd6-8bd1d9732f6e

📥 Commits

Reviewing files that changed from the base of the PR and between 7d0e596 and d51629d.

📒 Files selected for processing (1)
  • ark-core/src/ark_address.rs

@bonomat bonomat merged commit 07a8bca into master Mar 31, 2026
23 checks passed
@bonomat bonomat deleted the feat/Hash-eq-ark-address branch March 31, 2026 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants