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

[DRAFT, SEE TODO] Multi-domain WIP. #266

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

[DRAFT, SEE TODO] Multi-domain WIP. #266

wants to merge 2 commits into from

Conversation

robin-near
Copy link
Contributor

@robin-near robin-near commented Mar 18, 2025

TODO:

  • Add comments
  • Revise README
  • Add separate methods for public_key and latest_key_version since adding Option to empty function still breaks API.
  • Review my own code
  • Fix vote.rs integration tests (maybe in a future PR?)

Done:

  • Major logic implemented
  • All unit tests pass
  • Integration tests that are expected to pass do pass

ProtocolContractState::Resharing(state) => {
Ok(state.current_state.key_state.threshold())
Ok(state.resharing_key.proposed_parameters().threshold())
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this threshold come from the old participant set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? I don't know what threshold() is intended to be used for so it's kinda impossible to say.

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 it is used for voting on contract updates.

}
ProtocolContractState::Resharing(state) => {
AuthenticatedParticipantId::new(
state.resharing_key.proposed_parameters().participants(),
Copy link
Contributor

Choose a reason for hiding this comment

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

here too, we should not give the right to vote on contract updates to any participant not yet holding a valid keyshare.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I had a different thought but thinking about it again, yeah I think you're right.

Comment on lines +133 to +136
self.cleanup_if_timed_out();
let Some(instance) = self.instance.as_ref() else {
return Err(KeyEventError::NoActiveKeyEvent.into());
}
// Ensure the key_event_id matches
if self.current_key_event_id() != *key_event_id {
};
Copy link
Contributor

Choose a reason for hiding this comment

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

If the cleanup removes the timed out instance, this function will throw an error which is propagated back to the API call. This results in the contract state not changing.
Not throwing an error and persisting the state change would make it easier for the leader to notice when the instance timed out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes you're totally right. I'm just gonna not return error if there's no current instance. Nobody really cares about the return value anyway. And in tests we can always just look at the state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I'm still returning error. Because whether we remove the timed out instance has no bearing on anything. A timed out instance must always be treated by everyone as if the instance were removed. Actually removing it is just for the purpose of making the code that follows simpler.

@robin-near robin-near changed the title [DRAFT, DOES NOT COMPILE] Multi-domain WIP (it really is WIP). [DRAFT, SEE TODO] Multi-domain WIP. Mar 19, 2025
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.

2 participants