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

Cryp-153: Confidential asset creation #601

Closed
wants to merge 8 commits into from

Conversation

@ParnianAlimi
Copy link

@ParnianAlimi ParnianAlimi commented Sep 11, 2020

Implementation for registering confidential assets.

pallets/asset/src/ethereum.rs Outdated Show resolved Hide resolved
pallets/asset/src/lib.rs Outdated Show resolved Hide resolved
Portfolio::<T>::set_default_portfolio_balance(did, &ticker, total_supply);
}
<AssetOwnershipRelations>::insert(did, ticker, AssetOwnershipRelation::AssetOwned);
Self::deposit_event(RawEvent::AssetCreated(

This comment has been minimized.

@satyamakgec

satyamakgec Sep 15, 2020
Member

I think this also needs to be under if confidential condition.

This comment has been minimized.

@ParnianAlimi

ParnianAlimi Sep 15, 2020
Author

Good call.

This comment has been minimized.

@maxsam4

maxsam4 Sep 16, 2020
Member

@satyamakgec Why do you think so? There is nothing confidential in the event. Perhaps we can add a Confidential/Non-Confidential enum to the event but I see no reason to not emit an event here.

This comment has been minimized.

@maxsam4

maxsam4 Sep 16, 2020
Member

Ah, confidential lib is emitting a different event. I'd suggest that we use this event for both confidential and non-confidential assets with an enum differentiating between both.

This comment has been minimized.

@satyamakgec

satyamakgec Sep 16, 2020
Member

I did not want to emit that event to create a differentiation b/w confidential and non-confidential asset creation but if we choose the enum to represent the asset type then I think it makes sense and we can emit AssetCreated event in the creation of the confidential asset.

pallets/confidential-asset/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@aafshar-poly aafshar-poly left a comment

lgtm 👍

Portfolio::<T>::set_default_portfolio_balance(did, &ticker, total_supply);
}
<AssetOwnershipRelations>::insert(did, ticker, AssetOwnershipRelation::AssetOwned);
if !is_confidential {

This comment has been minimized.

@satyamakgec

satyamakgec Sep 16, 2020
Member

You can make it in the same if as order of storage changes doesn't matter here.

<AssetOwnershipRelations>::insert(did, ticker, AssetOwnershipRelation::AssetOwned);
if !is_confidential {
      <BalanceOf<T>>::insert(ticker, did, total_supply);
      Portfolio::<T>::set_default_portfolio_balance(did, &ticker, total_supply);
      Self::deposit_event(RawEvent::AssetCreated(
         did,
         ticker,
         total_supply,
         divisible,
         asset_type,
         did,
      ));
}
Copy link
Member

@maxsam4 maxsam4 left a comment

Some minor comments.

@@ -0,0 +1,149 @@
#![cfg_attr(not(feature = "std"), no_std)]

This comment has been minimized.

@maxsam4

maxsam4 Sep 16, 2020
Member

Best to move these to ./primitives/src. All our types are defined there.

Portfolio::<T>::set_default_portfolio_balance(did, &ticker, total_supply);
}
<AssetOwnershipRelations>::insert(did, ticker, AssetOwnershipRelation::AssetOwned);
if !is_confidential {

This comment has been minimized.

@maxsam4

maxsam4 Sep 16, 2020
Member

I think the event should be emitted even for confidential assets.

pallets/asset/src/lib.rs Show resolved Hide resolved
@ParnianAlimi
Copy link
Author

@ParnianAlimi ParnianAlimi commented Sep 21, 2020

I have to abandon this PR in favour of #623. This PR was changing the Settlement repo, which doesn't exist anymore. Other than that the content is identical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.