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(wallet): new command to publish a contract definition transaction #4133

Merged
merged 38 commits into from
May 26, 2022

Conversation

mrnaveira
Copy link
Contributor

@mrnaveira mrnaveira commented May 24, 2022

Description

  • Created a new transaction output features for the Contract Definition
    • For simplicity, I decided to leave out fields that could be more coupled to the template format.
    • The contract_id is calculated as a hash of the contents of the contract definition, in the line of what is described in the RFCs. I used ConsensusHashWriter to make it totally consistent.
    • I created a new Output Features version to signal the consensus breaking changes.
    • Had to update the genesis block struct to include the new features
    • Had to update the gRPC types to include the new features
  • Created a new command in the tari console wallet (publish-contract-definition):
    1. Reads the contract definition from a JSON file. I defined auxiliary structs (base_layer/wallet/src/assets/contract_definition.rs) to decouple the file format from the output features.
    2. Created a new transaction with the CONTRACT_DEFNITION flag and the new Contract Definition output features.
    3. Publishes the corresponding UTXO into the network.
  • Right now the base layer does not perform any check in the values in the contract definition. In the future we want to implement custom consensus rules for contract definition (like avoiding duplicates, spending requirements, etc.) but I purposely left that out of this PR for simplicity.

Motivation and Context

As a user, I want to be able to publish a new contract definition into the network through the console wallet.

How Has This Been Tested?

  • Created a new unit test for the consensus encoding of the new Contract Definition output features
  • Created a new integration test for the wallet CLI that reads the contract definition from a JSON file and successfully publishes the transaction

sdbondi
sdbondi previously approved these changes May 25, 2022
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Looks good - approved with few minor comments

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.

This looks good.
I just have an issue with the derive(Hash)
And a question about the one type

// Fixed lenght of all string fields in the contract definition
const STR_LEN: usize = 32;

#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, Hash)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq)]

This is not the same Hash trait we use, this is the rust one std::hash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it because output_features.rs (which is the "parent" struct) derives the standard that Hash trait, so it makes it required to do it here as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

mmm, wonder why it has it...
It looks like it could be removed, but I think that's for another PR

}
}

#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, Hash)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq)]

}
}

#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, Hash)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq)]

}
}

#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, Hash)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq)]

base_layer/wallet/src/assets/contract_definition.rs Outdated Show resolved Hide resolved
sdbondi
sdbondi previously approved these changes May 25, 2022
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

+1

@mrnaveira mrnaveira marked this pull request as draft May 25, 2022 11:00
@sdbondi sdbondi removed the mq-failed label May 25, 2022
@mrnaveira mrnaveira marked this pull request as ready for review May 25, 2022 11:59
stringhandler
stringhandler previously approved these changes May 26, 2022
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Nice. I want to note that the public functions may not look like that in the final form, but this is fine for a start

@aviator-app aviator-app bot removed the mq-failed label May 26, 2022
@aviator-app aviator-app bot merged commit b4991a4 into tari-project:development May 26, 2022
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.

5 participants