Skip to content
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

Add roles settings #246

Merged
merged 5 commits into from
Dec 1, 2024
Merged

Add roles settings #246

merged 5 commits into from
Dec 1, 2024

Conversation

matthme
Copy link
Contributor

@matthme matthme commented Nov 28, 2024

No description provided.

@matthme matthme requested a review from a team November 28, 2024 23:05
@@ -1071,22 +1070,22 @@ export class TryCpConductor implements IConductor {
const agent_key =
appForAgent.agentPubKey ??
(await this.adminWs().generateAgentPubKey());
const membrane_proofs = appForAgent.membraneProofs ?? {};
const roles_settings = appForAgent.rolesSettings;
Copy link

Choose a reason for hiding this comment

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

Is is safe to assume that appForAgent.roleSettings will not be null or undefined and we do not need a fallback value anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

undefined is a valid input to the InstallAppPayload so this should be fine either way.

{ progenitor: progenitorKey }
);
t.equal(provisionedCell.dna_modifiers.origin_time, originTime);
t.deepEqual(provisionedCell.dna_modifiers.quantum_time, quantumTime);
Copy link
Member

Choose a reason for hiding this comment

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

not sure if its overkill but should we add an assertion that the membrane proof is commited to our chain?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's overkill, because we're not testing the correctness of Holochain here.

Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

LGTM with suggestions

ts/src/types.ts Outdated Show resolved Hide resolved
ts/test/trycp/conductor.ts Outdated Show resolved Hide resolved
ts/test/local/scenario.ts Outdated Show resolved Hide resolved
{ progenitor: progenitorKey }
);
t.equal(provisionedCell.dna_modifiers.origin_time, originTime);
t.deepEqual(provisionedCell.dna_modifiers.quantum_time, quantumTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's overkill, because we're not testing the correctness of Holochain here.

@matthme
Copy link
Contributor Author

matthme commented Dec 1, 2024

Thank you! Addressed your suggestions @jost-s and will merge if the tests pass.

@matthme matthme merged commit 71ea66a into main Dec 1, 2024
2 checks passed
@matthme matthme deleted the roles-settings branch December 1, 2024 18:19
matthme added a commit that referenced this pull request Dec 1, 2024
* add new roles-settings field

* fix role name

* added changelog

* buump holochain_conductor_api

* address suggestions
matthme added a commit that referenced this pull request Dec 2, 2024
* Backport Add roles settings (#246)

* add new roles-settings field

* fix role name

* added changelog

* buump holochain_conductor_api

* address suggestions

* add missing import
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.

4 participants