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

[chain] Make LocalChain::apply_changeset infallible #1576

Open
ValuedMammal opened this issue Aug 27, 2024 · 4 comments
Open

[chain] Make LocalChain::apply_changeset infallible #1576

ValuedMammal opened this issue Aug 27, 2024 · 4 comments
Labels
api A breaking API change new feature New feature or request

Comments

@ValuedMammal
Copy link
Contributor

It seems like the only way this can fail is if we don't find a base AND extension doesn't include block 0, but I think one of these will have to be true, i.e. extension must have a genesis block or else we have a base.

for cp in init_cp.iter() {
if cp.height() >= start_height {
extension.insert(cp.height(), cp.hash());
} else {
base = Some(cp);
break;
}
}

let new_tip = match base {
Some(base) => base
.extend(extension.into_iter().map(BlockId::from))
.expect("extension is strictly greater than base"),
None => LocalChain::from_blocks(extension)?.tip(),
};

@ValuedMammal
Copy link
Contributor Author

Perhaps more importantly, this function actually allows you to change the genesis block if the changeset happens to include it (although why would it?) which seems incongruent with most other methods

@notmandatory
Copy link
Member

notmandatory commented Aug 30, 2024

Is this something you think should be done for the 1.0.0-beta milestone? would it help make any wallet user facing functions infallible ?

@notmandatory notmandatory added new feature New feature or request api A breaking API change labels Aug 30, 2024
@ValuedMammal
Copy link
Contributor Author

I don't think it's terribly urgent and seems like it mostly affects developers who are working with LocalChain. The issue was originally motivated by a discussion with @rustaceanrob who pointed out that MissingGenesisError is surprisingly common for something that should be an inherent property of a LocalChain object.

@rustaceanrob
Copy link
Contributor

What I mean by inherent or an invariant of LocalChain is if all constructors of LocalChain require a genesis block then calls of merging chains may only be fallible in that they do not connect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change new feature New feature or request
Projects
Status: Discussion
Development

No branches or pull requests

3 participants