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

Paratoken Standard #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Paratoken Standard #32

wants to merge 1 commit into from

Conversation

coinfork
Copy link

@coinfork coinfork commented Dec 1, 2021

Initial draft of the Paratoken standard

Initial draft of the Paratoken standard
Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

Some typos and suggestions.

- [Asset Identification](#asset-identification)
- [Paratoken Structure](#paratoken-structure)
- [Base Interface](#base-interface)
- [Common Types](#base-interface)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [Common Types](#base-interface)
- [Common Types](#common-types)

Comment on lines +72 to +73
The paratoken standard enforces the use of [`Universally Unique Asset Identifier`](uuaid/uuaid.md) as an
identifier of the asset on its API.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think uuaid/uuaid.md is included in the PSP, maybe add a hosted link?

Choose a reason for hiding this comment

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

I'm also curious to learn more about Universally Unique Asset Identifier

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this needs further clarification

/// owner.
fn create_asset( owner: Origin, policy: MonetaryPolicy);

/// Transfer the ownership of `asset` from `origin` to `target`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Transfer the ownership of `asset` from `origin` to `target`.
/// Transfer the ownership of `asset` from `origin` to `new_owner`.

/// Mints new `amount` of `asset` and transfer to `owner` account.
fn mint( owner: Origin, asset: T::AssetId, amount: T::AssetBalance);

/// Destroies `amount` from `token` of `asset`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Destroies `amount` from `token` of `asset`.
/// Destroys `amount` from `token` of `asset`.

// Balances and transfers
// ===================================

/// Transfers `amount` of `token` from `asset` from `from` account to `target` account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Transfers `amount` of `token` from `asset` from `from` account to `target` account.
/// Transfers `amount` of `token` from `asset` from `from` account to `to` account.

token: T::TokenId,
amount: T::AssetBalance);

/// Fetchs the balance of `tokens` of `asset` for `owner` account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Fetchs the balance of `tokens` of `asset` for `owner` account.
/// Fetches the balance of `tokens` of `asset` for `owner` account.


## Policies

Polices define what is the behaviour of the asset for specific operations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Polices define what is the behaviour of the asset for specific operations.
Policies define what is the behaviour of the asset for specific operations.

@Noc2
Copy link
Collaborator

Noc2 commented Dec 8, 2021

@coinfork Feel free to ping me here, once you are happy with this. I can merge the draft, so that it’s easier to share and potentially to get additional feedback.

@coinfork
Copy link
Author

@coinfork Feel free to ping me here, once you are happy with this. I can merge the draft, so that it’s easier to share and potentially to get additional feedback.

Sounds good! We'll be pushing a few updates first.


/// Only this issuer can mint tokens but just once.
/// That ensures that no more tokens will be created after the first minting.
OneTimeIssuer(AccountId),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is MintingPolicy mutable? If it is, then we don't need this. The asset owner can update the policy to NotAllowed after the minting is done.

Choose a reason for hiding this comment

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

At the moment, MintingPolicy can be both mutable and immutable to suggest that OneTimeIssuer is required.

token won't support any inflation.

```Rust
struct Mutability {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
struct Mutability {
enum Policy {

Otherwise it is very confusing

and maybe you will also want something like

enum Mutability {
    Immutable,
    Mutable { by: AccountId },
}

Choose a reason for hiding this comment

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

Scoped mutability is an interesting idea

Copy link

@cowboy-bebug cowboy-bebug Feb 15, 2022

Choose a reason for hiding this comment

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

Having the attribute of mutability for a policy is confusing to me. Stuff will happen and policies must be updated.
Could something like below considered (having a set of authorities)?

/// Authorities with privilege to update policy.
/// `None` means no one can update the policy making it immutable.
pub type PolicyMakers = Option<Vec<AccountId>>;

Some voting mechanism could also be implemented for PolicyMakers so it's "fair".

pub enum Event<T: Config> {
/// A new asset was created by `owner`.
/// \[ asset_id, owner \]
AssetCreated(T::AssetId, T::AccountId),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
AssetCreated(T::AssetId, T::AccountId),
AssetCreated { asset_id: T::AssetId, owner: T::AccountId },

Named fields are supported now, we might as well use it.

}
```

## Extension: Batch
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this should be in another meta operation standard?

Choose a reason for hiding this comment

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

What is a meta operation standard? @xlc Can you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean this doesn't really needs to be token related. For features like batch transaction, proxy, multisig etc, they could be in another standard for those utility/meta operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

You might be right that that is preferable.
Note, though, that the proposed batch transactions here allow for optimizations (e.g. optimized batch minting of NFTs) which generic batches (e.g. via pallet_utilitly::batch) would not.

Comment on lines +229 to +233
| 0x0001 | 1.0.0 | [Batch Extension](extensions/batch.md) |
| 0x0002 | 1.0.0 | [Chunks Extension](extensions/chunks.md) |
| 0x0003 | 1.0.0 | [Enumerable Extension](extensions/enumerable.md) |
| 0x0004 | 0.1.0 | **TBD** Teleportation |
| 0x0005 | 1.0.0 | [Operator](extensions/operator.md) |

Choose a reason for hiding this comment

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

I can't find extensions/*

Copy link

@bernardoaraujor bernardoaraujor Jan 29, 2022

Choose a reason for hiding this comment

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

after scrolling down I found the ## Extension: * sections of psp-32.md
wouldn't it make more sense to point at these sections, instead of extensions/*?


/// Creates a new asset using the given `policy` where `origin` will be the
/// owner.
fn create_asset( owner: Origin, policy: MonetaryPolicy);
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 return an AssetId? MonetaryPolicy is not defined.

amount: T::AssetBalance);

/// Fetchs the balance of `tokens` of `asset` for `owner` account.
fn balance(owner: Account, asset: T::Account, token: T::TokenId) -> T::AssetBalance;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between asset (T::Account?) and token here?

// ===================================

/// Returns the asset attribute of `key` for the given `asset`.
fn asset_attribute( asset: T::AssetId, key: T::AttributeKey) -> Option<T::AttributeValue>;
Copy link
Contributor

Choose a reason for hiding this comment

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

T::AttributeKey should be just AttributeKey, given that you define AttributeKey later on. I assume you intend this to be a shared/standardized type. The draft does not explain what "attributes" exactly are or for what purpose those should be used. You briefly touch on it in the summary at the beginning, but a (small) proper subsection would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, there are many assumptions about that generic type T. Should T implement a trait with specific associated types? All those associated types (Account, AssetId, AssetBalance) should probably be fixed and well defined anyway.

Comment on lines +72 to +73
The paratoken standard enforces the use of [`Universally Unique Asset Identifier`](uuaid/uuaid.md) as an
identifier of the asset on its API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this needs further clarification

@lamafab lamafab added the awaiting-changes Waiting for upstream changes label Mar 18, 2022
@lamafab
Copy link
Contributor

lamafab commented Mar 18, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-changes Waiting for upstream changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants