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.

Sign up for GitHub

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

Jump to bottom

pemissionless assets ED and ratio #3702

Closed
wants to merge 5 commits into from
Closed

pemissionless assets ED and ratio #3702

wants to merge 5 commits into from

Conversation

kkast
Copy link
Contributor

@kkast kkast commented Jun 16, 2023 •
edited by dzmitry-lahoda
Loading

⛓
Design Doc how to handle ED and Ratio Parameters of permissionless assets + minatable Cosmwasm assets

  • PR title is my best effort to provide summary of changes and has clear text to be part of release notes
  • I marked PR by misc label if it should not be in release notes
  • I have linked Zenhub/Github or any other reference item if one exists
  • I was clear on what type of deployment required to release my changes (node, runtime, contract, indexer, on chain operation, frontend, infrastructure) if any in PR title or description
  • I waited and did best effort for pr-workflow-check / draft-release-check to finish with success(green check mark) with my changes
  • I have added at least one reviewer in reviewers list
  • I tagged(@) or used other form of notification of one person who I think can handle best review of this PR
  • I have proved that PR has no general regressions of relevant features and processes required to release into production

dzmitry-lahoda
dzmitry-lahoda reviewed Jun 16, 2023
View reviewed changes
Copy link
Contributor

@dzmitry-lahoda dzmitry-lahoda left a comment

Choose a reason for hiding this comment

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

would be nice patch orml, and look what acala assets hub is using, and explain why new pallet and why we keed router?

other things are ok

dzmitry-lahoda
dzmitry-lahoda reviewed Jun 16, 2023
View reviewed changes
rfcs/0016-permissionless-assets-for-ibc-and-cw.md
#### orml tokens pallet EDs
Currently assets are being stored in orml tokens pallet which require Existential Deposits for stored assets.

**Potential Solution**: for non sufficient assets still provide non-zero Existential Deposit parameter which is similar to minimum balance in Asset Hub implementation. Then the Exitstential logic for orml tokens pallet will stay the same because account wont be able to hold a non sufficient asset without ED in parachain's native token. This will require a new field in AssetsRegistry: is_sufficient. All permissionless assets will have this field to be false by default. Assets created via permissioned extrinsic will allow to choose if asset is sufficient or not. Asset's sufficiency can be changed by governance.
Copy link
Contributor

Choose a reason for hiding this comment

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

AssetsRegistry: is_sufficient
should it be in assets registry or in assets? is_sufficient is required for all transactions, while symbol/name not. and foreign location only for xc tx.

i think is_sufficient should be moved as close as possible to transfer routines, ideally into orml tbh.

AR may not store field, but just deletage TX to update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Statemine has such a field in their assets pallet. I think it will be faster to handle is sufficient on our side than in ormls and we will control exact behavior we need. Ratio is None can be an alternative to deciding if asset is nonsufficient, i think we should keep ED to not depend on orml tokens in this implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah. good. thank you

@dzmitry-lahoda
Copy link
Contributor

dzmitry-lahoda commented Jun 16, 2023 •
edited
Loading

@kkast please

  1. write down what each pallet does oneline and what other pallets it use
  2. can do it via diagram (graph, compoents, etc)
  3. we should avoid cyclical dependency throw away hell and over complicated design (think about poor QA)
    ^^ super importatnt
  4. please checkout https://moonbeam.network/education/moonbeam-routed-liquidity/
  5. please checkout

blasrodri
blasrodri reviewed Jun 16, 2023
View reviewed changes
rfcs/0016-permissionless-assets-for-ibc-and-cw.md

### ED Manipulation

Existential Deposit(ED) is a safeguard against bloating storage of the chain. If a user transfers a nonvaluable asset and sets its ED to 0 or 1, this user will be able to affect performance of the parachain by transferring this asset to many accounts. Thus, to avoid this potential attack vector, we will use Asset Hub (formerly Statemine) approach of non-sufficient assets: a user to be able to recieve permissionless asset needs to hold ED in PICA on Picasso and LAYR on Composable parachains. This approach will limit the growth of the storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this solution

@dzmitry-lahoda dzmitry-lahoda self-requested a review June 17, 2023 22:56
dzmitry-lahoda
dzmitry-lahoda previously approved these changes Jun 17, 2023
View reviewed changes
@dzmitry-lahoda
Copy link
Contributor

ok, but please do not create new pallet, and plese think to make is suffcient storage in assets, not in asset registrya sit will make code simpler imho.

@dzmitry-lahoda
dz/242
0ab86d1 
Signed-off-by: dzmitry-lahoda <[email protected]>
@dzmitry-lahoda
Copy link
Contributor

@kkast please take a look into commit about XCM/IBC permissonless asset creation. Are we ok with that?

@dzmitry-lahoda dzmitry-lahoda self-requested a review July 10, 2023 14:39
@dzmitry-lahoda
Copy link
Contributor

closing in favor of #3777

@dzmitry-lahoda dzmitry-lahoda removed their request for review July 12, 2023 21:04
@dzmitry-lahoda dzmitry-lahoda removed their assignment Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josietyleung josietyleung josietyleung left review comments

@blasrodri blasrodri blasrodri left review comments

@dzmitry-lahoda dzmitry-lahoda dzmitry-lahoda left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kkast @dzmitry-lahoda @blasrodri @josietyleung