Refactor the DNA Properties and how Applet Configurations are Created to Allow Adding of Additional Configs from Wizard #21

tatssato · 2023-01-16T06:40:24Z

This pr exposes add_config zome function which takes in additional sensemaker config (same format) and creates necessary entries out of that config. get_configs zome function is also exposed to allow anyone in the neighborhood to read existing configurations.

julioholon

Looks good to me, all tests pass.

julioholon · 2023-01-16T13:41:20Z

dnas/sensemaker/zomes/coordinator/sensemaker/src/init.rs


 #[hdk_extern]
 pub fn init(_: ()) -> ExternResult<InitCallbackResult> {
    let prop = Properties::get()?;
    let my_key = agent_info()?.agent_latest_pubkey;
-    if let false = Properties::is_community_activator(my_key)? {
+    if let false = Properties::is_community_activator(my_key.clone())? {


I would rename this variable "false" to something more in line with the check you are doing, like is_community_activator

Oh if let is actually a rust sugar syntax for handling conditional statements and false is actually one of the possible values of the statement on the right Properties::is_community_activator.

If it is confusing, I could use a normal conditional statement handling instead @julioholon

weswalla · 2023-01-17T17:20:27Z

We should consolidate applet config registration (#18) with adding additional config, or clearly distinguish the difference between these two processes. The idea with #18 is when a member adds an applet to the NH, the applet will need to register sensemaker types (dimension, methods, contexts, etc.) that it depends on to be able to function properly.

Th differences I see between register_applet and add_config are the input type, indexing/linking the entries, and who is allowed to do it (currently anyone can register an applet, although perhaps we want to change this):

pub struct AppletConfigInput {
    pub name: String,
    // pub ranges: Vec<Range>, // leaving out ranges since this is not an entry and is just part of the dimension
    pub dimensions: Vec<Dimension>,
    // the base_type field in ResourceType needs to be bridged call
    pub resource_types: Vec<ConfigResourceType>,
    pub methods: Vec<ConfigMethod>,
    pub cultural_contexts: Vec<ConfigCulturalContext>,
}

pub struct RawSensemakerConfig {
    pub neighbourhood: String,
    pub wizard_version: String,
    pub config_version: String,
    pub creator: String,
    // pub ranges: Vec<Range>, // leaving out ranges since this is not an entry and is just part of the dimension
    pub dimensions: Vec<Dimension>,
    // the base_type field in ResourceType needs to be bridged call
    pub resources: Vec<ConfigResourceType>,
    pub methods: Vec<ConfigMethod>,
    pub contexts: Vec<ConfigCulturalContext>,
}

I assume that RawSensemakerConfig is more like a "global" config (at lease the first 3 properties: neighbourhood, wizard_version, config_version) and these would apply to all applets in this NH (so no need to include these properties in AppletConfigInput) Although I see how having a creator property on the applet config would be useful to know (especially if we want them to have certain access rights other than the CA).

It would be nice to be able to "intersect" these types like in TS, for example:

pub struct AppletConfigInput {
    pub name: String,
    // pub ranges: Vec<Range>, // leaving out ranges since this is not an entry and is just part of the dimension
    pub dimensions: Vec<Dimension>,
    // the base_type field in ResourceType needs to be bridged call
    pub resource_types: Vec<ConfigResourceType>,
    pub methods: Vec<ConfigMethod>,
    pub cultural_contexts: Vec<ConfigCulturalContext>,
}

pub struct RawSensemakerConfig {
    pub neighbourhood: String,
    pub wizard_version: String,
    pub config_version: String,
} & AppletConfigInput

And for indexing, a sensemaker config is linked off the CA pub key, whereas applet config is uses a path like "all_applets.${applet_name}" to index and keep track of all registered applets. Other than that, it would be nice to reuse the function create_entries_from_config as to not duplicate the same code for both applet and sensemaker config.

weswalla · 2023-01-17T17:18:14Z

dnas/sensemaker/zomes/coordinator/sensemaker/src/utils.rs

+        .dimensions
+        .clone()
+        .into_iter()
+        .map(|dimension: Dimension| create_entry(&EntryTypes::Dimension(dimension)))


instead of using create_entry directly, it would be better to use create_dimension and the other create_{sensemaker_type} functions, so that if we want to change how these entries are indexed we only have to change it in one spot.

Yes definitely. Thanks for pointing me to it. Will use that instead :D

weswalla · 2023-01-18T17:41:22Z

@tatssato I added ts definitions of config related types

julioholon

I think having a bunch of AppletConfigs that can be merged into a full SenseMakerConfig makes sense, and it also makes sense to store what changes each applet made to the config, so it can be "undone" sif the NH CA decides to remove that Applet.

julioholon · 2023-01-19T15:12:00Z

types/src/applet.ts

    name: string,
-    // pub ranges: Array<Range>, // leaving out ranges since this is not an entry and is just part of the dimension


This was one of the feedbacks we received from Damien, to have Ranges as entries. I think @tatssato is working on coding this in, no? @tatssato do you remember why we needed Ranges as entries?

It was so they could be fetched (separately from a dimension) as well as reused.

Yup exactly what @weswalla said. I will be working on this in a separate PR including other recommendations Damien had :D

@tatssato note that there is a PR for beta-rc.2 with passing local tests but something about nix-shell in github CI isn't working currently, you may want to work off a branch of that, or merge it into your current branch!
#22

@weswalla Thanks. Got it. Will merge the beta-rc.2 to this branch :D

weswalla · 2023-01-19T16:54:41Z

I think having a bunch of AppletConfigs that can be merged into a full SenseMakerConfig makes sense, and it also makes sense to store what changes each applet made to the config, so it can be "undone" sif the NH CA decides to remove that Applet.
@julioholon

I'm not sure if it makes sense actually to have one full sensemaker config object that has to get updated everytime a new applet is added, as handling updates to an entry can be tricky and branches (of an update tree) can occur. If a UI wants a singular master config object, that can simply be a derived object (constructed in the zome function).

I think what is most important is that we can fetch all sensemaker related objects (ranges, dimensions, methods, contexts, etc.) and know where they came from (an applet, or directly configured to the sensemaker by the CA). We can use links to accomplish this, which I think is sufficient for our use-cases.

…configuration

tatssato · 2023-01-25T09:25:04Z

@weswalla @julioholon
Finished implementing all feedbacks so far. Have some comments and questions but I think we can already merge this PR.

Tests are passing locally, but CI tests are failing as mentioned by @weswalla here

P.S. I already merged the beta-rc.2 branch but it might be safe to just merge the PR for it first before merging this one :D

Here are some notes

Changes

the DNA property of Sensemaker DNA changed. It now takes in two fields. sensemaker_config field takes in SensemakerConfig. The applet_configs field takes in a vector of AppletConfig for any number of applets that are already installed prior to the Sensemaker DNA being installed.
move AppletConfigInput to integrity zome. This avoids integrity zome depending on coordinator zome
exclude the creator key from AppletConfig given that this is visible through Action
add update_sensemaker_config function and remove add_config function.
add get_latest_sensemaker_config function. Removed get_configs function.
modify create_entries_from_record to use already existing functions to create the entries from config

New validations requried (will include this in the PR including the changes from damien's reviews)

only community activator can create the link from agent to sensemaker config
only community activator can create the link from "all_applets" path to applet name/hash
only community activator can create the link from applet name/hash path to AppletConfig
only ca can create AppletConfig
only ca can create SensemakerConfig
The same SensemakerConfig entry is updated which results in faster read of Sensemaker Config
The community_activator key should stay consistent accross updates of SensemakerConfig entry

Let me know if any of you have any questions :D

weswalla

Looks great!

I would say ready to merge, but after we merge in the other PR #22

weswalla · 2023-01-25T21:21:05Z

dnas/sensemaker/zomes/coordinator/sensemaker/src/sensemaker_config.rs

+use sensemaker_integrity::{LinkTypes, Properties, SensemakerConfig};
+
+#[derive(Serialize, Deserialize, Debug)]
+pub struct UpdateConfigurtionInput {


typo: UpdateConfigurtionInput -> UpdateConfigurationInput

weswalla · 2023-01-25T21:34:11Z

dnas/sensemaker/zomes/coordinator/sensemaker/src/init.rs

+    // for each config, check the format and creat the config entries with the helper function
+    for config in prop.applet_configs {
+        config.clone().check_format()?;
+        create_entries_from_applet_config(config)?;


Ok so I see that you've consolidated the process of adding config items (resource types, ranges, dimensions, methods, etc.) into the AppletConfig type. I don't see any issues with this approach, however, it makes me think about to what extent do we want to distinguish config specifically added because of installing an applet (registering an applet) vs generic config added by the CA either at sensemaker spawning or sometime afterwards that is not for any specific applet.

Perhaps we would use a keyword as a applet name when adding config not from applet registration, which could be wrapped in a new zome function add_sensemaker_config, when a CA wants to, for example, add new dimension. Or something like that. I don't think these are critical, blocking questions right now, but how we answer them will affect how we index/access data from the sensemaker to display in the dashboard.

weswalla · 2023-01-26T16:20:27Z

@tatssato merged in develop branch which had been updated with the beta-rc.2 branch, CI should pass, and also updated some type definitions and got rid of serializeHash() and replaced with the equivalent function from @holochain/client

tatssato added 2 commits January 16, 2023 14:34

implement add_config function

1d364d2

fmt

b13f38b

tatssato changed the base branch from main to develop January 16, 2023 06:41

tatssato linked an issue Jan 16, 2023 that may be closed by this pull request

Allow adding of additional configuration #20

Closed

julioholon approved these changes Jan 16, 2023

View reviewed changes

weswalla reviewed Jan 17, 2023

View reviewed changes

weswalla added 2 commits January 18, 2023 11:28

Merge branch 'develop' into load-additional-configuration

08db913

include sensemaker config types in ts type defs

0dd8060

julioholon reviewed Jan 19, 2023

View reviewed changes

tatssato added 5 commits January 24, 2023 16:01

Merge remote-tracking branch 'origin/beta-rc.2' into load-additional-…

c72699b

…configuration

fix test

9ddedd0

separate applet and sensemaker config

4b8f02d

fix test

b1b02f0

test passing for the new configuration format

4f9ab0e

tatssato changed the title ~~Implement add_config Zome Function to Allow Adding of Additional Configs from Wizard~~ Refactor the DNA Properties and how Applet Configurations are Created to Allow Adding of Additional Configs from Wizard Jan 25, 2023

weswalla approved these changes Jan 25, 2023

View reviewed changes

weswalla added 3 commits January 26, 2023 10:04

Merge branch 'develop' into load-additional-configuration

6051615

encodeHashToBase64 instead of serializeHash

6be2da1

update type definitions

1bf88fd

fix typo

a9dc905

tatssato merged commit e009b64 into develop Jan 27, 2023

tatssato deleted the load-additional-configuration branch January 27, 2023 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the DNA Properties and how Applet Configurations are Created to Allow Adding of Additional Configs from Wizard #21

Refactor the DNA Properties and how Applet Configurations are Created to Allow Adding of Additional Configs from Wizard #21

tatssato commented Jan 16, 2023

julioholon left a comment

julioholon Jan 16, 2023

tatssato Jan 18, 2023

weswalla commented Jan 17, 2023 •

edited

Loading

weswalla Jan 17, 2023

tatssato Jan 18, 2023

weswalla commented Jan 18, 2023

julioholon left a comment

julioholon Jan 19, 2023

weswalla Jan 19, 2023 •

edited

Loading

tatssato Jan 20, 2023

weswalla Jan 20, 2023 •

edited

Loading

tatssato Jan 22, 2023

weswalla commented Jan 19, 2023

tatssato commented Jan 25, 2023 •

edited

Loading

weswalla left a comment

weswalla Jan 25, 2023

weswalla Jan 25, 2023

weswalla commented Jan 26, 2023

		name: string,
		// pub ranges: Array<Range>, // leaving out ranges since this is not an entry and is just part of the dimension

Refactor the DNA Properties and how Applet Configurations are Created to Allow Adding of Additional Configs from Wizard #21

Refactor the DNA Properties and how Applet Configurations are Created to Allow Adding of Additional Configs from Wizard #21

Conversation

tatssato commented Jan 16, 2023

julioholon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswalla commented Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswalla commented Jan 18, 2023

julioholon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswalla Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswalla Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswalla commented Jan 19, 2023

tatssato commented Jan 25, 2023 • edited Loading

Changes

New validations requried (will include this in the PR including the changes from damien's reviews)

weswalla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswalla commented Jan 26, 2023

weswalla commented Jan 17, 2023 •

edited

Loading

weswalla Jan 19, 2023 •

edited

Loading

weswalla Jan 20, 2023 •

edited

Loading

tatssato commented Jan 25, 2023 •

edited

Loading