Ref docs frame currency #2683

juangirini · 2023-12-11T17:41:52Z

polkadot address: 12zshjtBEmK43evpHZXyQEVHkfVhW7J5sUvs64EmusSCxPUp

docs/sdk/src/reference_docs/frame_currency.rs

liamaharon

I think it's worth defining a clear goal of what this ref doc should achieve, then outlining the sections and writing each sentence to ensure they are maximally fulfilling this goal.

I understood it as something to disambiguate the different traits, trait implementations, and terminology we have for handling tokens. We should be careful not to let the scope creep and start writing documentation for the actual traits / implementations here.

docs/sdk/src/reference_docs/frame_currency.rs

juangirini · 2023-12-14T13:05:33Z

I think it's worth defining a clear goal of what this ref doc should achieve, then outlining the sections and writing each sentence to ensure they are maximally fulfilling this goal.

I understood it as something to disambiguate the different traits, trait implementations, and terminology we have for handling tokens. We should be careful not to let the scope creep and start writing documentation for the actual traits / implementations here.

Indeed, we have different goals in mind for this document. We definitely don't want to talk (only) about the Currency trait here, but what is Currency in FRAME if not a very important use case of the fungible traits?
What would be a valid structure for this document from your point of view?

liamaharon · 2023-12-15T09:47:29Z

what is Currency in FRAME if not a very important use case of the fungible traits?

Not sure what you mean here?

Indeed, we have different goals in mind for this document.

What is/was your goal for this doc?

What would be a valid structure for this document from your point of view?

Primarily I think the goal of an overarching doc for token traits and trait impls should be to allow a reader with zero prior knowledge of Substrate come away with

A good understanding of the patterns used for tokens in Substrate (traits and trait impls), and why we use these patterns
A good understanding of which traits and pallets exist, the differences between the traits and pallets, and how to choose a trait or pallet for their use case

This could look something loosely like

## Tokens in Substrate

briefly explains 
- how Substrate follows the pattern of traits and trait implementation for tokens
- why this approach was chosen

### Fungible Traits and Pallets

list and compare fungible traits and pallets

### Non-fungible Traits and Pallets

list and compare non-fungible traits and pallets

I don't think this needs to be long. Each sentence should be genuinely informative and contribute to the goal of the document.

Let me know what you think, I'm open to other ideas.

juangirini · 2023-12-15T14:24:20Z

what is Currency in FRAME if not a very important use case of the fungible traits?

Not sure what you mean here?

This is supposed to be a Currency documentation, being currency a fungible token I think we should talk about both of them.

Indeed, we have different goals in mind for this document.

What is/was your goal for this doc?

Give a high level overview of currency and fungible traits.

## Tokens in Substrate

briefly explains 
- how Substrate follows the pattern of traits and trait implementation for tokens
- why this approach was chosen

### Fungible Traits and Pallets

list and compare fungible traits and pallets

### Non-fungible Traits and Pallets

list and compare non-fungible traits and pallets

I don't think we should generalize it to "all" tokens, and therefore I wouldn't touch NFT in the Currency documentation, that should belong somewhere else.
The current PR already explains the Fungible Traits and Pallets part, this could of course improved and adapted.
I like the idea of the first part you propose as long as we stick to currency

how Substrate follows the pattern of traits and trait implementation for tokens

why this approach was chosen
WDYT? Do you want to add that to the PR?

juangirini · 2023-12-15T15:49:43Z

The more broader/generic tokens document will be taken care of in paritytech/polkadot-sdk-docs#70

paritytech-cicd-pr · 2023-12-15T15:54:26Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4739247

JuaniRios · 2023-12-19T16:37:57Z

Just wanted to comment that we spent 2 days researching the move from currency to fungible/fungibles, and it was very hard to understand.
This doc will definitely help us and other people in the future :)

Our main pain points that we had to clear up:

Do freezes make the same changes to `AccountData` storage as locks?

Yes

Should all previous locks be freezes, and all reserves be now holds?

No, because previously a lock could assume that the free balance wouldn't be reduced below that amount. But now if a hold happens, then you are considering also the frozen/locked balance, which might lead to unexpected behaviour.
This is why pallet-staking had to go from lock to hold

If a pallet used a currency trait, can another pallet use a fungible/fungibles trait?

Depends very much on the expected behavior. For example if one pallet is using locks, but you are ok with that amount being considered in another pallet's hold, then you could do that.
In our case we could use pallet-vesting which uses LockableCurrency, because if we were to replace it with the fungible trait it would be a Freeze (we don't mind having a hold on the frozen amount). And from our understanding freezes and locks produce the same outcome on the AccountData storage.

What are the bookkeeping functions mentioned by gav?

In here and here Gav explains that since a frozen balance cannot guarantee that the free will ever go below that (due to slashing on holds), then we should add a bookkeeping function which deals with the case where we find less free balance than the frozen one.
We are not sure what this means or how it would look like in the code.

It is mentioned that this is implemented in the conviction-voting pallet, but we couldn't find where that happened.

Why is the output of reducable_balance when the parameter fortitude=Polite calculated like that?

Here Liam's visualisation came in handy.

I think we would benefit from also explaining how the currency and fungible traits relate to the AccountData storage.

If I got anything wrong from my self-answered questions, please let me know :)

juangirini · 2023-12-22T15:56:08Z

Hey @JuaniRios thanks for your valuable input. We've loosely spoken about fungible/s traits in this doc as there's another one more specific to it paritytech/polkadot-sdk-docs#70. I am cc'ing @liamaharon as he's working on it

liamaharon · 2023-12-24T07:10:57Z

Thank you @JuaniRios, this is super useful! I have a draft PR for token doc here #2802, will add a TODO there to make sure all these points have been covered. If you have any other suggestions for these docs you can comment in that pr :)

docs/sdk/src/reference_docs/frame_currency.rs

kianenigma · 2024-01-08T16:06:58Z

docs/sdk/src/reference_docs/frame_currency.rs

+//! financial operations. The key functions of
+//! [`pallet_balances`](../../../pallet_balances/index.html) include transferring tokens between
+//! accounts, checking account balances, and adjusting balances for staking or fees. This pallet
+//! implements the [`fungible`](../../../frame_support/traits/tokens/fungible/index.html)


I think the most important detail came last; What I think is the most important piece of info here is that pallet-balances and pallet-assets provide implementations for all of the aforementioned traits. We can list those here (while linking to the proper impl block in the rust-docs).

kianenigma · 2024-01-08T16:07:11Z

docs/sdk/src/reference_docs/frame_currency.rs

+//! still using the [`Currency`](../../../frame_support/traits/tokens/currency/index.html) trait.
+//! There one could find the relevant code examples for upgrading.
+//!
+//! ## Fungible Traits


You are not going to briefly talk about Fungibles?

I see fungibles making sense in multiple assets scenario but not here, there is this other PR that covers more generic tokens topics

kianenigma · 2024-01-08T16:07:44Z

substrate/frame/support/src/traits/tokens/fungible/freeze.rs

@@ -15,7 +15,26 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

+//! # Freeze


This is indeed the right place to document this. Any methods that can also be improved?

kianenigma · 2024-01-08T16:08:55Z

substrate/frame/support/src/traits/tokens/fungible/freeze.rs

+//! |__on_hold__|__ed__|  
+//! ```
+//!
+//! Freezes require a `Reason`, which is configurable and is expected to be an enum aggregated


Possibly mention or link to the exact keyword of RuntimeFreezeReason.

kianenigma

This is a good start, but I expect a lot more substance on this particular topic. I expect this to be in #2802? if so, we can merge this already.

juangirini · 2024-01-09T14:23:33Z

This is a good start, but I expect a lot more substance on this particular topic. I expect this to be in #2802? if so, we can merge this already.

Yes, we have split the topic into two sections, this one focused only on "currency" as a native token, and a more generic one covered by that other PR

gpestana · 2024-01-09T20:44:14Z

docs/sdk/src/reference_docs/frame_currency.rs

-//! Notes:
+//! Currency related traits in FRAME provide standardized interfaces for implementing various
+//! currency functionalities. The fundamental reason to have these traits is to allow pallets to
+//! utilize a token without relying on the implementation detail. Or else we could have a perfectly


Suggested change

//! utilize a token without relying on the implementation detail. Or else we could have a perfectly

//! utilize tokens without relying on the implementation detail. Or else we could have a perfectly

gpestana · 2024-01-09T20:45:15Z

docs/sdk/src/reference_docs/frame_currency.rs

-//! Notes:
+//! Currency related traits in FRAME provide standardized interfaces for implementing various
+//! currency functionalities. The fundamental reason to have these traits is to allow pallets to
+//! utilize a token without relying on the implementation detail. Or else we could have a perfectly


Or else we could have a perfectly fine runtime without these traits.

I don't quite get what this sentence means to say. Could you elaborate/rephrase?

It should be removed as it is redundant, what I meant is that you don't need these traits if your runtime doesn't require a currency

gpestana · 2024-01-09T20:47:05Z

docs/sdk/src/reference_docs/frame_currency.rs

+//! currency functionalities. The fundamental reason to have these traits is to allow pallets to
+//! utilize a token without relying on the implementation detail. Or else we could have a perfectly
+//! fine runtime without these traits.
+//! These traits enable developers to create, transfer, and manage different forms of digital assets


different forms of digital assets

Nit but the first thing I thought when I read this was NFTs and I believe the Currency traits are not the best abstractions.

gpestana · 2024-01-09T20:48:40Z

docs/sdk/src/reference_docs/frame_currency.rs

-//! 	- This footgun: <https://github.com/paritytech/polkadot-sdk/pull/1900#discussion_r1363783609>
+//! ## The Currency Trait
+//!
+//! The [`Currency`][4] trait was initially introduced in Substrate to manage the native token


[Currency][4]

Nit but this is the first time I'm seeing this type of referencing in our docs, maybe we should just add the URL in line? I find it easier to just click on the word/expression directly if I want more details.

Yes, it's a tradeoff... this format helps readability when there are long / too many links
#2683 (comment)

gpestana · 2024-01-09T20:50:08Z

docs/sdk/src/reference_docs/frame_currency.rs

+//! approach for single-asset operations. Fungible is currently preferred over currency as the
+//! latter is deprecated.
+//!
+//! #### Holds and Freezes


Suggested change

//! #### Holds and Freezes

//! ### Holds and Freezes

gpestana · 2024-01-09T20:51:18Z

docs/sdk/src/reference_docs/frame_currency.rs

+//!
+//! Learn more about this two concepts in
+//! [`frame_support::traits::tokens::fungible::hold`][1] and
+//! [`frame_support::traits::tokens::fungible::freeze`][2].


Suggested change

//! [`frame_support::traits::tokens::fungible::freeze`][2].

//! [`frame_support::traits::tokens::fungible::freeze`].

I believe the link is added automatically in this case?

Not really as frame_support is not a dependency of polkadot-sdk-docs

Should be able to add it as a dep?

gpestana · 2024-01-09T20:51:59Z

docs/sdk/src/reference_docs/frame_currency.rs

+//! [`frame_support::traits::tokens::fungible::freeze`][2].
+//!
+//! ## Balances Pallet
+//! The [`pallet_balances`](../../../pallet_balances/index.html) is a key component in FRAME. It


This linking looks better to me, maybe we can stick with that and remove the references in the other cases?

I have removed them where they made the lines too long or difficult to read

gpestana · 2024-01-09T20:56:54Z

docs/sdk/src/reference_docs/frame_currency.rs

@@ -1,8 +1,55 @@
-//! FRAME Currency Abstractions and Traits
+//! # FRAME Currency Abstractions and Traits


I think it would be useful to have a table with the comparing the Currency vs Fungible/s terms and a description on how they differ (e.g. locks vs holds, etc). This is usually a place where people get confused and it would be great if we could have a single place to refresh the memory.

I agree we can improve this document by adding some more information, maybe we can keep that for another PR, wdyt?

kianenigma · 2024-02-16T09:08:41Z

@juangirini I wonder if you intend to finish the last bits of this, as there seems to be little left? else we can consider mentoring someone else to finish it.

In case you do, feel free to leave your DOT address in the PR description :)

juangirini · 2024-03-08T14:47:13Z

@juangirini I wonder if you intend to finish the last bits of this, as there seems to be little left? else we can consider mentoring someone else to finish it.

In case you do, feel free to leave your DOT address in the PR description :)

@kianenigma, yes I'll finish the PR, there is very little left to do. Thanks for the reminder

juangirini · 2024-03-12T14:07:47Z

@gpestana I have addressed your comments on this other PR as I don't have write access anymore here
cc @liamaharon @kianenigma

kianenigma · 2024-03-18T20:43:44Z

@liamaharon is looking into how this PR can be incorporated into his more recent and broader PR around tokens, but nonetheless submitting a tip for gratitude.

kianenigma · 2024-03-18T20:43:50Z

/tip medium

substrate-tip-bot · 2024-03-18T20:44:17Z

@kianenigma A referendum for a medium (80 DOT) tip was successfully submitted for @juangirini (12zshjtBEmK43evpHZXyQEVHkfVhW7J5sUvs64EmusSCxPUp on polkadot).

Referendum number: 585.

substrate-tip-bot · 2024-03-18T20:44:54Z

The referendum has appeared on Polkassembly.

liamaharon · 2024-03-20T09:24:15Z

Closing, will absorb into #2802

Closes paritytech/polkadot-sdk-docs#70 WIP PR for an overview of how to develop tokens in FRAME. - [x] Tokens in Substrate Ref Doc - High-level overview of the token-related logic in FRAME - Improve docs with better explanation of how holds, freezes, ed, free balance, etc, all work - [x] Update `pallet_balances` docs - Clearly mark what is deprecated (currency) - [x] Write fungible trait docs - [x] Evaluate and if required update `pallet_assets`, `pallet_uniques`, `pallet_nfts` docs - [x] Absorb #2683 - [x] Audit individual trait method docs, and improve if possible Feel free to suggest additional TODOs for this PR in the comments --------- Co-authored-by: Bill Laboon <laboon@users.noreply.github.com> Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Sebastian Kunert <skunert49@gmail.com>

Closes paritytech/polkadot-sdk-docs#70 WIP PR for an overview of how to develop tokens in FRAME. - [x] Tokens in Substrate Ref Doc - High-level overview of the token-related logic in FRAME - Improve docs with better explanation of how holds, freezes, ed, free balance, etc, all work - [x] Update `pallet_balances` docs - Clearly mark what is deprecated (currency) - [x] Write fungible trait docs - [x] Evaluate and if required update `pallet_assets`, `pallet_uniques`, `pallet_nfts` docs - [x] Absorb paritytech#2683 - [x] Audit individual trait method docs, and improve if possible Feel free to suggest additional TODOs for this PR in the comments --------- Co-authored-by: Bill Laboon <laboon@users.noreply.github.com> Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Sebastian Kunert <skunert49@gmail.com>

juangirini added 2 commits December 8, 2023 16:04

initial currency outline

8c94c41

intial frame currency

c94350a

juangirini requested a review from liamaharon December 11, 2023 17:41

juangirini commented Dec 11, 2023

View reviewed changes

docs/sdk/src/reference_docs/frame_currency.rs Outdated Show resolved Hide resolved

juangirini commented Dec 11, 2023

View reviewed changes

docs/sdk/src/reference_docs/frame_currency.rs Outdated Show resolved Hide resolved

liamaharon reviewed Dec 14, 2023

View reviewed changes

reorganize fungibles docs

14f11bb

juangirini requested a review from liamaharon December 14, 2023 17:22

juangirini mentioned this pull request Dec 15, 2023

Tokens in Substrate Docs paritytech/polkadot-sdk-docs#70

Closed

remove fungibles and assets

9a94042

juangirini self-assigned this Dec 15, 2023

juangirini added R0-silent Changes should not be mentioned in any release notes T11-documentation This PR/Issue is related to documentation. labels Dec 15, 2023

juangirini marked this pull request as ready for review December 15, 2023 15:52

juangirini requested review from a team December 15, 2023 15:52

Merge branch 'master' into jg/ref-docs-frame-currency

3fe82bc

liamaharon mentioned this pull request Dec 24, 2023

Tokens in FRAME Docs #2802

Merged

6 tasks

juangirini added 3 commits December 26, 2023 14:47

Merge branch 'master' into jg/ref-docs-frame-currency

cbb7291

fix rustdocs links

6babc75

fix rustdocs links

933a8e2

kianenigma reviewed Jan 8, 2024

View reviewed changes

docs/sdk/src/reference_docs/frame_currency.rs Outdated Show resolved Hide resolved

kianenigma reviewed Jan 8, 2024

View reviewed changes

docs/sdk/src/reference_docs/frame_currency.rs Outdated Show resolved Hide resolved

kianenigma reviewed Jan 8, 2024

View reviewed changes

docs/sdk/src/reference_docs/frame_currency.rs Outdated Show resolved Hide resolved

kianenigma reviewed Jan 8, 2024

View reviewed changes

docs/sdk/src/reference_docs/frame_currency.rs Outdated Show resolved Hide resolved

kianenigma reviewed Jan 8, 2024

View reviewed changes

kianenigma approved these changes Jan 8, 2024

View reviewed changes

review improvements

80ff2ce

juangirini requested a review from a team January 9, 2024 14:13

gpestana reviewed Jan 9, 2024

View reviewed changes

kianenigma mentioned this pull request Feb 16, 2024

[FRAME] Currency::transfer() can create locks if used incorrectly #3332

Open

juangirini mentioned this pull request Mar 12, 2024

apply review comments #3627

Closed

liamaharon closed this Mar 20, 2024

	//! utilize a token without relying on the implementation detail. Or else we could have a perfectly
	//! utilize tokens without relying on the implementation detail. Or else we could have a perfectly

	//! [`frame_support::traits::tokens::fungible::freeze`][2].
	//! [`frame_support::traits::tokens::fungible::freeze`].

		@@ -1,8 +1,55 @@
		//! FRAME Currency Abstractions and Traits
		//! # FRAME Currency Abstractions and Traits

Ref docs frame currency #2683

Ref docs frame currency #2683

Conversation

juangirini commented Dec 11, 2023 • edited

liamaharon left a comment

Choose a reason for hiding this comment

juangirini commented Dec 14, 2023

liamaharon commented Dec 15, 2023

juangirini commented Dec 15, 2023

juangirini commented Dec 15, 2023

paritytech-cicd-pr commented Dec 15, 2023

JuaniRios commented Dec 19, 2023 • edited

Do freezes make the same changes to AccountData storage as locks?

Should all previous locks be freezes, and all reserves be now holds?

If a pallet used a currency trait, can another pallet use a fungible/fungibles trait?

What are the bookkeeping functions mentioned by gav?

Why is the output of reducable_balance when the parameter fortitude=Polite calculated like that?

juangirini commented Dec 22, 2023 • edited

liamaharon commented Dec 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma Jan 8, 2024 • edited

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

juangirini commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juangirini Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma commented Feb 16, 2024

juangirini commented Mar 8, 2024

juangirini commented Mar 12, 2024

kianenigma commented Mar 18, 2024

kianenigma commented Mar 18, 2024

substrate-tip-bot bot commented Mar 18, 2024

substrate-tip-bot bot commented Mar 18, 2024

liamaharon commented Mar 20, 2024 • edited

juangirini commented Dec 11, 2023 •

edited

JuaniRios commented Dec 19, 2023 •

edited

Do freezes make the same changes to `AccountData` storage as locks?

juangirini commented Dec 22, 2023 •

edited

liamaharon commented Dec 24, 2023 •

edited

kianenigma Jan 8, 2024 •

edited

juangirini Mar 8, 2024 •

edited

liamaharon commented Mar 20, 2024 •

edited