Skip to content

Move MsgVerify from oracles to proto#468

Merged
michaeldjeffrey merged 24 commits intomasterfrom
connor/move-msg-verify
Dec 2, 2025
Merged

Move MsgVerify from oracles to proto#468
michaeldjeffrey merged 24 commits intomasterfrom
connor/move-msg-verify

Conversation

@connormck333
Copy link
Contributor

@connormck333 connormck333 commented Nov 7, 2025

MsgVerify trait originally belonged to oracles/file_store but was moved to oracles/file_store_oracles to remove the helium_proto dependency on file_store. This meant that cdr_service depended on file_store_oracles as it also uses MsgVerify.

We don't want to move MsgVerify to helium-proto as that would add a dependency on helium-crypto. Instead, I created a new trait MsgHasSignature which will be added to all protos with a signature field. MsgVerify has been moved to helium-wallet-rs where it will add a verify() method to all protos with a MsgHasSignature trait.

@madninja
Copy link
Member

madninja commented Nov 7, 2025

I am not a big fan of this change. We use helium/
-lib to tie together libraries at a higher level. Let’s have it there

@michaeldjeffrey
Copy link
Contributor

I am not a big fan of this change. We use helium/ -lib to tie together libraries at a higher level. Let’s have it there

I don't think it belongs in helium-lib. This verification is specific to prost messages.
Putting it in helium-lib would mean adding prost over there. And adding some features to exclude solana deps.

@madninja
Copy link
Member

I am not a big fan of this change. We use helium/ -lib to tie together libraries at a higher level. Let’s have it there

I don't think it belongs in helium-lib. This verification is specific to prost messages. Putting it in helium-lib would mean adding prost over there. And adding some features to exclude solana deps.

After a bit of discussion, I think we came to the proposal to add a helium-proto-ext workspace member to the helium-wallet-rs repo.. that way helium-lib and the new proto-ext crate share the same crypto and proto versions, while allowing other crates to pick up the helper functions without getting into solana dependencies

@connormck333
Copy link
Contributor Author

I am not a big fan of this change. We use helium/ -lib to tie together libraries at a higher level. Let’s have it there

I don't think it belongs in helium-lib. This verification is specific to prost messages. Putting it in helium-lib would mean adding prost over there. And adding some features to exclude solana deps.

After a bit of discussion, I think we came to the proposal to add a helium-proto-ext workspace member to the helium-wallet-rs repo.. that way helium-lib and the new proto-ext crate share the same crypto and proto versions, while allowing other crates to pick up the helper functions without getting into solana dependencies

We had another chat about this change. If we were to keep MsgVerify in helium-proto, then we can use a proc macro to detect the signature field and automatically have the verify method. It also saves us from having to update helium-lib everytime we add a new proto and want the verify method. Let us know what you think.

kurotych and others added 6 commits November 14, 2025 16:47
* Added historical gateway info req and service

* Added v1 to historical gateway req

* Updated formatting for info historical rpc

* Changed historical info to info_at_timestamp

* Add speedtest_value_out_of_bounds field to speedtest_verification_result (#457)

---------

Co-authored-by: Anatolii Kurotych <akurotych@gmail.com>
@connormck333 connormck333 marked this pull request as ready for review November 17, 2025 10:50
connormck333 and others added 2 commits November 25, 2025 12:35
@michaeldjeffrey michaeldjeffrey merged commit a585fb2 into master Dec 2, 2025
7 checks passed
@michaeldjeffrey michaeldjeffrey deleted the connor/move-msg-verify branch December 2, 2025 19:17
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.

5 participants