-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
4 zome re structure and crate docs #89
base: main
Are you sure you want to change the base?
Conversation
@@ -6,42 +6,74 @@ opt-level = "z" | |||
|
|||
[workspace] | |||
resolver = "2" | |||
members = ["crates/*"] | |||
members = ["crates/*/coordinator","crates/*/integrity"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this pattern not imply that holoom_types
, jaq_wrapper
, shared_utils
aren't workspace members?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are dependency packages .. i dont think they need to be members.
[workspace.dependencies.username_registry_validation] | ||
path = "crates/username_registry_validation" | ||
[workspace.dependencies.evm_attestation_coordinator] | ||
path = "crates/evm_attestation/coordinator" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
evm_attestation_*
is a misleading name. It should probably be evm_signing_offer_*
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i used the term for a package name to refer to all EVM operations that would imply some kind of attestation.. we can change the package name
path = "crates/holoom_recipe/coordinator" | ||
|
||
[workspace.dependencies.holoom_recipe_integrity] | ||
path = "crates/holoom_recipe/integrity" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason for adding holoom_
to the name of recipe related crates, but not the others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me They are meta operations .. you could create recipes for anything... but here we are creating recipes specifically for holoom crates/packages
let zome_name = zome_info()?.name; | ||
if my_pubkey == authority_agent { | ||
functions.insert(( | ||
zome_name.clone(), "ingest_signed_username".into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not relevant to this zome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right that one might be a mistake..
Ok(ValidateCallbackResult::Valid) | ||
} | ||
#[hdk_extern] | ||
pub fn validate(op: Op) -> ExternResult<ValidateCallbackResult> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to find a way to generalise this since it's repeated so many times now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is another one we have to swallow.. as every zome has these common validation functions. trying to separate them from the zomes validation files.. wont work and breaks modularity.
// .await | ||
// .unwrap(); | ||
//} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated again?
pub fn sign_message(message: SignableBytes) -> ExternResult<Signature> { | ||
sign(agent_info()?.agent_initial_pubkey, message) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is signing now part of username_registry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you can see the sign_message is two lines of code.. I did not feel it merited a separate package.. and just added the function to wallet and username attestation zomes, where it is used
.await | ||
.unwrap(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-/
let zome_name = zome_info()?.name; | ||
if my_pubkey == authority_agent { | ||
} | ||
functions.insert((zome_name, "recv_remote_signal".into())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think wallet attestations doesn't need this
crates/shared_utils/src/ping.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping needs to be a coordinator zome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my rule of thumb is not to make a zome out of anything that doesnt have an integrity part to it.. i dont see the point of making 'ping' a zome.. i would add it as a function to an existing zome or many zomes.
No description provided.