Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

Fix loading #1836

Merged
merged 10 commits into from
Nov 7, 2019
Merged

Fix loading #1836

merged 10 commits into from
Nov 7, 2019

Conversation

lucksus
Copy link
Collaborator

@lucksus lucksus commented Nov 6, 2019

PR summary

Loading of instances was broken.

Two reasons why we didn't notice this right away:

  1. The test for this was not really testing loading and got deactivated. Which then allowed a change to happen that broken the loading code in every case.
  2. The conductor always first tries to load from the given storage. If that fails, it creates a new instance. This second branch obviously doesn't erase everything in the CAS and was ending up with a partially loaded state. Source chain was loaded correctly. Agent keys are stored in the keystore anyway. So we only missed some things, especially the holding list.

changelog

  • if this is a code change that effects some consumer (e.g. zome developers) of holochain core, then it has been added to our between-release changelog with the format
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)

documentation

@lucksus lucksus added the NEEDS REVIEW This PR requires peer code review label Nov 6, 2019
Comment on lines 14 to 18
pub async fn initialize(
instance: &Instance,
dna: Option<Dna>,
maybe_dna: Option<Dna>,
context: Arc<Context>,
) -> HcResult<Arc<Context>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if we have a Dna in the context/state, but then call initialize with Some(dna)? If that is invalid behavior, maybe that is an argument for splitting this into two different functions so we can't do something invalid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please have a look again. I've left it as one function for now, but added comments and a panic in case maybe_dna is Some AND there is a DNA already in the nucleus state.

Copy link
Collaborator

@maackle maackle left a comment

Choose a reason for hiding this comment

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

Cool, fair enough

@lucksus lucksus merged commit b311228 into develop Nov 7, 2019
@neonphog neonphog deleted the fix-loading branch March 5, 2020 23:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NEEDS REVIEW This PR requires peer code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants