refactor: rename verification message #1637

Wazzymandias · 2024-01-31T00:42:28Z

Motivation

Adding additional verification message types add a lot of complexity
However, existing verification message type is tightly coupled to Ethereum
Rename verification message type for better extensibility for future changes

Change Summary

Rename verification message type for better extensibility for future changes

Merge Checklist

Choose all relevant options below by adding an x now or at any time before submitting for review

PR title adheres to the conventional commits standard
PR has a changeset
PR has been tagged with a change label(s) (i.e. documentation, feature, bugfix, or chore)
PR includes documentation if necessary.
All commits have been signed

Additional Context

If this is a relatively large or complex change, provide more details here that will help reviewers

PR-Codex overview

This PR focuses on renaming the verification message from "VERIFICATION_ADD_ETH_ADDRESS" to "VERIFICATION_ADD_ADDRESS".

Detailed summary

Renamed VERIFICATION_ADD_ETH_ADDRESS to VERIFICATION_ADD_ADDRESS in multiple files.
Updated related functions and type definitions accordingly.
Added new dependencies in package.json.

The following files were skipped due to too many changes: packages/core/src/protobufs/typeguards.ts, apps/hubble/src/rpc/server.ts, packages/core/src/crypto/eip712.ts, packages/core/src/signers/testUtils.ts, yarn.lock, packages/core/src/builders.ts, apps/hubble/src/storage/engine/index.ts, apps/hubble/src/rpc/test/httpServer.test.ts, apps/hubble/src/rpc/test/verificationService.test.ts, apps/hubble/src/storage/db/migrations/4.uniqueVerifications.test.ts, apps/replicator/src/util.ts, protobufs/schemas/message.proto, apps/replicator/src/processors/verification.ts, packages/core/src/builders.test.ts, packages/core/src/validations.ts, apps/hubble/src/storage/engine/index.test.ts, packages/core/src/factories.ts, packages/core/src/validations.test.ts, apps/hubble/src/storage/stores/verificationStore.test.ts, apps/hubble/src/storage/stores/verificationStore.ts, packages/hub-web/src/generated/message.ts, packages/hub-nodejs/src/generated/message.ts, packages/core/src/protobufs/generated/message.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

…the future

changeset-bot · 2024-01-31T00:42:31Z

🦋 Changeset detected

Latest commit: 1b57489

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@farcaster/hub-nodejs	Minor
@farcaster/hub-web	Minor
@farcaster/replicator	Minor
@farcaster/core	Minor
@farcaster/hubble	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2024-01-31T00:42:33Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
hub-monorepo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 31, 2024 5:27am

socket-security · 2024-01-31T00:44:00Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/[email protected]	None	`0`	9.44 kB	junderw
npm/[email protected]	None	`+1`	15.7 kB	junderw

View full report↗︎

codecov · 2024-01-31T04:04:27Z

Codecov Report

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

Comparison is base (80c8f7d) 73.75% compared to head (1b57489) 73.78%.

Files	Patch %	Lines
packages/core/src/validations.ts	84.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1637      +/-   ##
==========================================
+ Coverage   73.75%   73.78%   +0.02%     
==========================================
  Files          99       99              
  Lines        9000     9002       +2     
  Branches     1998     1999       +1     
==========================================
+ Hits         6638     6642       +4     
+ Misses       2244     2242       -2     
  Partials      118      118

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

sanjayprabhu

Generally looks good. Left some comments. I think biggest thing is we should keep this PR to be a pure rename refactoring. And add the sol changes in a followup

sanjayprabhu · 2024-01-31T04:01:55Z

protobufs/schemas/message.proto

+/** Adds a Verification of ownership of an Address based on Protocol */
+message VerificationAddAddressBody {
+  bytes address = 1; // Address being verified for a given Protocol
+  bytes protocol_signature = 2; // Signature produced by the user's address for a given Protocol


maybe we should calls this user_signature?

yeah i wasn't sure what's best to name this

since we have the user signature for FID, i wanted something to help distinguish the two

open to user_signature if we feel it won't clash with FID signature verification

Hmm, fair. Maybe address_verification_signature or user_address_signature?

i like address_verification_signature - do we want it in this PR or can it be separate?

I've made the update on the followup branch built on top of this one: https://github.com/farcasterxyz/hub-monorepo/tree/feat/solana-verifications

This is perhaps a silly question, but are these changes backwards compatible with the old message format? eth_signature is referenced in a lot of places in code—will that suddenly start breaking for old messages when someone updates their hub-nodejs package?

Or will the code generated by protobuf libraries automatically support both calls?

Hmm, it would break. Consumers of the library would have to rename.
But maybe we can add a function to the class in hub-nodejs called eth_signature that returns the eth_signature if protocol == ethereurm and throws otherwise?

protobufs/schemas/message.proto

packages/core/src/verifications.ts

packages/core/src/validations.ts

sds

Overall looks great. I always wondered why we insisted on tying it to ETH and not just have a verification_type field.

sds · 2024-01-31T19:46:02Z

protobufs/schemas/message.proto

+/** Adds a Verification of ownership of an Address based on Protocol */
+message VerificationAddAddressBody {
+  bytes address = 1; // Address being verified for a given Protocol
+  bytes protocol_signature = 2; // Signature produced by the user's address for a given Protocol


This is perhaps a silly question, but are these changes backwards compatible with the old message format? eth_signature is referenced in a lot of places in code—will that suddenly start breaking for old messages when someone updates their hub-nodejs package?

Or will the code generated by protobuf libraries automatically support both calls?

sanjayprabhu

What is this file apps/hubble/src/rustfunctions.test.ts ?

sanjayprabhu · 2024-01-31T20:12:54Z

packages/core/src/validations.ts

@@ -623,7 +620,12 @@ export const validateVerificationAddEthAddressBody = async (
 export const validateVerificationRemoveBody = (
  body: protobufs.VerificationRemoveBody,
 ): HubResult<protobufs.VerificationRemoveBody> => {
-  return validateEthAddress(body.address).map(() => body);
+  switch (body.protocol) {
+    case protobufs.Protocol.ETHEREUM:


We should have this in the validate Add as well

sanjayprabhu · 2024-01-31T20:15:51Z

protobufs/schemas/message.proto

+/** Adds a Verification of ownership of an Address based on Protocol */
+message VerificationAddAddressBody {
+  bytes address = 1; // Address being verified for a given Protocol
+  bytes protocol_signature = 2; // Signature produced by the user's address for a given Protocol


Hmm, it would break. Consumers of the library would have to rename.
But maybe we can add a function to the class in hub-nodejs called eth_signature that returns the eth_signature if protocol == ethereurm and throws otherwise?

Wazzymandias · 2024-01-31T21:24:57Z

What is this file apps/hubble/src/rustfunctions.test.ts ?

@sanjayprabhu Some file aditya and cassie set up - I used it to test signature verification for phantom wallet

sanjayprabhu · 2024-01-31T21:34:10Z

Ah, weird, I don't know why github thinks it's a binary file.

This reverts commit fd9f9ff.

* Revert "refactor: rename verification message (#1637)" This reverts commit fd9f9ff. * Add changeset

Wazzymandias added 2 commits January 30, 2024 18:32

feat: rename verification message to support additional protocols in …

74c19f0

…the future

chore: add changeset

3e90808

Wazzymandias requested a review from sanjayprabhu January 31, 2024 00:42

vercel bot deployed to Preview January 31, 2024 03:27 View deployment

refactor: change signature for verification message

1afc542

Wazzymandias force-pushed the refactor/verification-message-rename branch from 3386e23 to 1afc542 Compare January 31, 2024 03:59

vercel bot deployed to Preview January 31, 2024 04:00 View deployment

sanjayprabhu reviewed Jan 31, 2024

View reviewed changes

vercel bot deployed to Preview January 31, 2024 04:35 View deployment

Wazzymandias force-pushed the refactor/verification-message-rename branch from 4bc362c to c65cbba Compare January 31, 2024 04:37

vercel bot deployed to Preview January 31, 2024 04:38 View deployment

Wazzymandias force-pushed the refactor/verification-message-rename branch from c65cbba to 51d8967 Compare January 31, 2024 04:40

vercel bot deployed to Preview January 31, 2024 04:40 View deployment

Wazzymandias force-pushed the refactor/verification-message-rename branch from 51d8967 to 9add14d Compare January 31, 2024 04:55

vercel bot deployed to Preview January 31, 2024 04:56 View deployment

Wazzymandias force-pushed the refactor/verification-message-rename branch from 9add14d to 52fa50d Compare January 31, 2024 04:58

vercel bot deployed to Preview January 31, 2024 04:59 View deployment

Wazzymandias force-pushed the refactor/verification-message-rename branch from 52fa50d to 38e145d Compare January 31, 2024 05:18

vercel bot deployed to Preview January 31, 2024 05:18 View deployment

remove solana related changes

1b57489

Wazzymandias force-pushed the refactor/verification-message-rename branch from 38e145d to 1b57489 Compare January 31, 2024 05:27

vercel bot deployed to Preview January 31, 2024 05:27 View deployment

Wazzymandias marked this pull request as ready for review January 31, 2024 05:30

Wazzymandias requested a review from sanjayprabhu January 31, 2024 05:32

Wazzymandias mentioned this pull request Jan 31, 2024

feat: solana verifications #1638

Closed

5 tasks

sds reviewed Jan 31, 2024

View reviewed changes

sanjayprabhu reviewed Jan 31, 2024

View reviewed changes

sanjayprabhu approved these changes Jan 31, 2024

View reviewed changes

Wazzymandias merged commit fd9f9ff into main Jan 31, 2024
10 checks passed

Wazzymandias deleted the refactor/verification-message-rename branch January 31, 2024 21:35

sanjayprabhu added a commit to sanjayprabhu/hub-monorepo that referenced this pull request Feb 2, 2024

Revert "refactor: rename verification message (farcasterxyz#1637)"

16bdf9d

This reverts commit fd9f9ff.

sanjayprabhu added a commit that referenced this pull request Feb 2, 2024

fix: Revert verification rename (#1642)

f8c5f28

* Revert "refactor: rename verification message (#1637)" This reverts commit fd9f9ff. * Add changeset

sanjayprabhu mentioned this pull request Feb 6, 2024

feat: Rename verifications message to support multiple protocols #1667

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rename verification message #1637

refactor: rename verification message #1637

Wazzymandias commented Jan 31, 2024 •

edited by pr-codex bot

Loading

changeset-bot bot commented Jan 31, 2024 •

edited

Loading

vercel bot commented Jan 31, 2024 •

edited

Loading

socket-security bot commented Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

sanjayprabhu left a comment

sanjayprabhu Jan 31, 2024

Wazzymandias Jan 31, 2024

sanjayprabhu Jan 31, 2024

Wazzymandias Jan 31, 2024

Wazzymandias Jan 31, 2024

sds Jan 31, 2024

sanjayprabhu Jan 31, 2024

sds left a comment

sds Jan 31, 2024

sanjayprabhu left a comment

sanjayprabhu Jan 31, 2024

sanjayprabhu Jan 31, 2024

Wazzymandias commented Jan 31, 2024

sanjayprabhu commented Jan 31, 2024

refactor: rename verification message #1637

refactor: rename verification message #1637

Conversation

Wazzymandias commented Jan 31, 2024 • edited by pr-codex bot Loading

Motivation

Change Summary

Merge Checklist

Additional Context

PR-Codex overview

Detailed summary

changeset-bot bot commented Jan 31, 2024 • edited Loading

🦋 Changeset detected

vercel bot commented Jan 31, 2024 • edited Loading

socket-security bot commented Jan 31, 2024 • edited Loading

codecov bot commented Jan 31, 2024 • edited Loading

Codecov Report

sanjayprabhu 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

Choose a reason for hiding this comment

sds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjayprabhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wazzymandias commented Jan 31, 2024

sanjayprabhu commented Jan 31, 2024

Wazzymandias commented Jan 31, 2024 •

edited by pr-codex bot

Loading

changeset-bot bot commented Jan 31, 2024 •

edited

Loading

vercel bot commented Jan 31, 2024 •

edited

Loading

socket-security bot commented Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading