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

feat(core): adds constitution UTXO features #4121

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented May 20, 2022

Description

  • adds ContractConstitution and related structs
  • implements consensus de/encoding for ConsensusConstitution
  • implements ConsensusEncoding ,Deref and DerefMut for MaxSizeVec
  • adds check for extraneous bytes to check_consensus_encoding_correctness

Motivation and Context

Add initial structs for contract constitution UTXO. Adding this to OutputFeatures is left for a subsequent PR.

How Has This Been Tested?

Unit tests for consensus encoding

@sdbondi sdbondi force-pushed the core-add-constitution-structure branch from 50be94b to 55eb217 Compare May 20, 2022 11:31
SWvheerden
SWvheerden previously approved these changes May 23, 2022
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

LGTM,
I only left two general questions but it good is it stands

@@ -67,6 +67,11 @@ pub struct OutputFeatures {
#[serde(default)]
pub recovery_byte: u8,
pub metadata: Vec<u8>,
// TODO: Add these fields
// pub contract_id: Option<FixedHash>,
// pub constitution: Option<ContractConstitution>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to add flags for these as well?
Or are we going with the presence of them is enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

OutputFlags already exist for the constitution. Whenever a contract_id is present we likely want one of those flags to be set

Comment on lines +176 to +177
pub max_new_committee_members: u32,
/// The maximum number of committee members that can be evicted.
pub max_evicted_committee_members: u32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps more options are better, but I don't see any reason why we would limit these two fields

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah wasn't sure what settings we want to add to this. Should only have settings that are useful since each setting translates into more validation complexity and impact performance. Suggestions welcome. Could also leave this out for now.

@sdbondi sdbondi force-pushed the core-add-constitution-structure branch from 55eb217 to af4f8ae Compare May 23, 2022 12:24
@aviator-app aviator-app bot merged commit da5696a into tari-project:development May 25, 2022
@sdbondi sdbondi deleted the core-add-constitution-structure branch May 25, 2022 11:19
aviator-app bot pushed a commit that referenced this pull request May 26, 2022
Description
---
Based on #4122 #4129 #4121 

- adds `SideChainFeatures` to `OutputFeatures`
- updates code and tests
- updates cucumber output feature consensus encoding
- allows MaxSizeVec to be used directly in consensus encoded structs

Motivation and Context
---
Partial implementation of RFC 312 contract constitution

How Has This Been Tested?
---
- New consensus de/encoding tests
- Manually on igor
- cucumbers pass
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.

3 participants