Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Add support for creating confidential assets. #623
Conversation
ParnianAlimi
requested review from
aafshar-poly,
satyamakgec,
maxsam4,
Centril and
vkomenda
| None, | ||
| Some(total_supply), | ||
| total_supply, | ||
| ); |
satyamakgec
Sep 22, 2020
Member
We don't need this at the time of the creation of the confidential asset. We are not minting anything here.
We don't need this at the time of the creation of the confidential asset. We are not minting anything here.
ParnianAlimi
Sep 22, 2020
Author
Once Arash's code goes in, we will be calling this function with total_supply = 0, so it won't really do anything meaningful, but I'll make it more explicit.
Once Arash's code goes in, we will be calling this function with total_supply = 0, so it won't really do anything meaningful, but I'll make it more explicit.
ParnianAlimi
Sep 22, 2020
Author
Thinking more about this, are you sure we don't need to update stats here? we are still adding a primary issuer for the asset and that should increment the number of investors of the asset.
Thinking more about this, are you sure we don't need to update stats here? we are still adding a primary issuer for the asset and that should increment the number of investors of the asset.
satyamakgec
Sep 23, 2020
Member
We are not really assigning the balance to the primary issuer so it doesn't make sense to call this.
We are not really assigning the balance to the primary issuer so it doesn't make sense to call this.
satyamakgec
Sep 23, 2020
Member
But we can capture this in another story if you want without delaying this PR approval.
But we can capture this in another story if you want without delaying this PR approval.
|
LGTM, just a few suggestions. |
|
|
||
| /// List of Tickers of the type ConfidentialAsset. | ||
| /// () -> List of confidential tickers | ||
| pub ConfidentialTickers get(fn confidential_tickers): Vec<AssetId>; |
vkomenda
Sep 22, 2020
Collaborator
Any access would require decoding the entire Vec. There is a way around that, just use a characteristic function instead - a map from AssetId to bool.
BTreeSet would be better than Vec: log(n) reads as opposed to n. But again it would require reading the whole BTreeSet from runtime storage, which can be avoided.
Any access would require decoding the entire Vec. There is a way around that, just use a characteristic function instead - a map from AssetId to bool.
BTreeSet would be better than Vec: log(n) reads as opposed to n. But again it would require reading the whole BTreeSet from runtime storage, which can be avoided.
satyamakgec
Sep 22, 2020
Member
@aafshar-poly & I had a discussion about this couple of weeks ago. Arash do you guys still need the whole vector of asset_ids to validate the proof? I think it was more related to the index of asset_id. we can re-discuss this.
@aafshar-poly & I had a discussion about this couple of weeks ago. Arash do you guys still need the whole vector of asset_ids to validate the proof? I think it was more related to the index of asset_id. we can re-discuss this.
ParnianAlimi
Sep 22, 2020
Author
Yes, we need the whole AssetID vector. The list of all confidential asset IDs ever created to be exact.
Yes, we need the whole AssetID vector. The list of all confidential asset IDs ever created to be exact.
vkomenda
Sep 23, 2020
Collaborator
OK, got you. The only place I found in in this PR where confidential_tickers is called is an application of AccountCreator::create. It takes a reference to the Vec. I would suggest modifying AccountCreator::create to take &[Scalar] instead for uniformity and also because slices are better as arguments as opposed to &Vec<_>.
OK, got you. The only place I found in in this PR where confidential_tickers is called is an application of AccountCreator::create. It takes a reference to the Vec. I would suggest modifying AccountCreator::create to take &[Scalar] instead for uniformity and also because slices are better as arguments as opposed to &Vec<_>.
|
LGTM, a few suggestions. |
| { | ||
| type Event: From<Event> + Into<<Self as frame_system::Trait>::Event>; | ||
| pub trait Trait: frame_system::Trait + IdentityTrait + BalancesTrait { | ||
| type Asset: AssetTrait<Self::Balance, Self::AccountId>; |
vkomenda
Sep 23, 2020
Collaborator
In general it would be good to have doc comments both on type parameters and the trait itself.
In general it would be good to have doc comments both on type parameters and the trait itself.
| valid_asset_ids.clone(), | ||
| ) | ||
| .unwrap(); | ||
| let valid_asset_ids = ConfidentialAsset::confidential_tickers(); |
vkomenda
Sep 23, 2020
Collaborator
Unused variable.
Unused variable.
ParnianAlimi
Sep 23, 2020
Author
it's used on line 242.
it's used on line 242.
|
|
||
| /// List of Tickers of the type ConfidentialAsset. | ||
| /// () -> List of confidential tickers | ||
| pub ConfidentialTickers get(fn confidential_tickers): Vec<AssetId>; |
vkomenda
Sep 23, 2020
Collaborator
OK, got you. The only place I found in in this PR where confidential_tickers is called is an application of AccountCreator::create. It takes a reference to the Vec. I would suggest modifying AccountCreator::create to take &[Scalar] instead for uniformity and also because slices are better as arguments as opposed to &Vec<_>.
OK, got you. The only place I found in in this PR where confidential_tickers is called is an application of AccountCreator::create. It takes a reference to the Vec. I would suggest modifying AccountCreator::create to take &[Scalar] instead for uniformity and also because slices are better as arguments as opposed to &Vec<_>.
|
Some minor nit-picks if you want to taker care before the merge |
| None, | ||
| Some(total_supply), | ||
| total_supply, | ||
| ); |
satyamakgec
Sep 23, 2020
Member
We are not really assigning the balance to the primary issuer so it doesn't make sense to call this.
We are not really assigning the balance to the primary issuer so it doesn't make sense to call this.
| None, | ||
| Some(total_supply), | ||
| total_supply, | ||
| ); |
satyamakgec
Sep 23, 2020
Member
But we can capture this in another story if you want without delaying this PR approval.
But we can capture this in another story if you want without delaying this PR approval.
| Self::funding_round(ticker), | ||
| total_supply, | ||
| Some(did), | ||
| )); |
satyamakgec
Sep 23, 2020
Member
Realistically we can cover this in the above if statement
Realistically we can cover this in the above if statement
Addresses CRYP-153. Adds an extrinsic to register a confidential asset.