Skip to content

feat: add on chain metrics for sign request payload version#2179

Merged
gilcu3 merged 5 commits intomainfrom
dsharifi/add-contract-metridcs
Feb 19, 2026
Merged

feat: add on chain metrics for sign request payload version#2179
gilcu3 merged 5 commits intomainfrom
dsharifi/add-contract-metridcs

Conversation

@DSharifi
Copy link
Contributor

@DSharifi DSharifi commented Feb 19, 2026

Summary

Closes #2178

@claude
Copy link

claude bot commented Feb 19, 2026

PR title type suggestion: This PR is adding new user-facing functionality (on-chain metrics), so the type prefix should be feat:.

Suggested title: feat: add on-chain metrics for sign request payload version

@claude
Copy link

claude bot commented Feb 19, 2026

Code Review

One issue found:

  • Unsafe arithmetic on u64 counters (crates/contract/src/lib.rs:214,218): The += 1 operations on sign_with_v1_payload_count and sign_with_v2_payload_count violate the project's "Use safe arithmetic methods" engineering standard. While overflow at u64::MAX is practically unreachable, the convention exists to avoid using primitive arithmetic operators. Consider using saturating_add:

    if request.deprecated_payload.is_some() {
        self.metrics.sign_with_v1_payload_count = 
            self.metrics.sign_with_v1_payload_count.saturating_add(1);
    }
    
    if request.payload_v2.is_some() {
        self.metrics.sign_with_v2_payload_count = 
            self.metrics.sign_with_v2_payload_count.saturating_add(1);
    }

Everything else looks good: state migration defaults counters to zero correctly, the DTO mapping is straightforward, tests follow the project's Given/When/Then convention, and the metrics() view method is properly exposed.

⚠️ (minor issue found)

@claude
Copy link

claude bot commented Feb 19, 2026

PR title type suggestion: This PR adds new on-chain metrics functionality to the contract, so the type prefix should be feat: instead of having no prefix.

Suggested title: feat: add on-chain metrics for sign request payload version

@DSharifi DSharifi changed the title Add on-chain metrics for sign request payload version chore: add on chain metrics for sign request payload version Feb 19, 2026
@claude
Copy link

claude bot commented Feb 19, 2026

PR title type suggestion: This PR adds new on-chain metrics tracking (payload version counts), which is a new user-facing capability. The type prefix should probably be feat: instead of chore:.

Suggested title: feat: add on-chain metrics for sign request payload version

@DSharifi DSharifi changed the title chore: add on chain metrics for sign request payload version feat: add on chain metrics for sign request payload version Feb 19, 2026
Copy link
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

I'm confused why we're adding these metrics in the contract. Shouldn't nodes keep track of this instead?

netrome
netrome previously approved these changes Feb 19, 2026
Copy link
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Edit: Discussed on slack. Since these are public methods we want to deprecate it's nice to have these metrics visible.

gilcu3
gilcu3 previously approved these changes Feb 19, 2026
Copy link
Contributor

@gilcu3 gilcu3 left a comment

Choose a reason for hiding this comment

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

Thank you!

@DSharifi DSharifi dismissed stale reviews from gilcu3 and netrome via 2c73ad4 February 19, 2026 13:53
@DSharifi DSharifi enabled auto-merge February 19, 2026 13:55
@DSharifi DSharifi added this pull request to the merge queue Feb 19, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 19, 2026
@gilcu3 gilcu3 added this pull request to the merge queue Feb 19, 2026
Merged via the queue into main with commit 34848e3 Feb 19, 2026
10 checks passed
@gilcu3 gilcu3 deleted the dsharifi/add-contract-metridcs branch February 19, 2026 14:51
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.

Add on-chain metrics for sign request payload version usage

3 participants