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

feat: add native token support for fungibles api #147

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

chungquantin
Copy link
Collaborator

@chungquantin chungquantin commented Jul 30, 2024

Remaining TODOs

  • Migrate from single Asset instance to multiple asset kinds using NativeOrWithId
  • Migrate existing methods:
    • transfer()
    • transfer_from() (throw UnsupportedMethod)
    • approve() (throw UnsupportedMethod)
    • increase_allowance() (throw UnsupportedMethod)
    • decrease_allowance() (throw UnsupportedMethod)
  • Write the custom benchmarking function for the fungible api method that support the native token
  • Refactor POP API to support native token
  • Migrate read state methods
  • Refactor contract integration tests and pallet unit testing to cover the native token
    • transfer()
    • transfer_from() (throw UnsupportedMethod)
    • approve() (throw UnsupportedMethod)
    • increase_allowance() (throw UnsupportedMethod)
    • decrease_allowance() (throw UnsupportedMethod)

Concerns

Right now, the pallet is a tight coupled because it needs both pallet-assets and pallet-balances to add the native token to the fungible pallet functionalities.

However, we need create a shared type for the pallet_balances::Balance and pallet_assets::Balance because the BalanceOf is used for parameter like amount in transfer() method. My implementation right now is to use of UnionOf and NativeOrWithId to provide the loose coupling to support type conversion but it would make the Fungibles pallet code mixes between loose coupling and tight coupling.

Daanvdplas and others added 30 commits May 19, 2024 17:38
# This is the 1st commit message:

refactor: general

# This is the commit message #2:

init

# This is the commit message #3:

begin refactor

# This is the commit message #4:

refactor: error handling

# This is the commit message #5:

tests: add error handling tests

# This is the commit message #6:

WIP

# This is the commit message #7:

finalise error handling

# This is the commit message #8:

refactor: easier review
Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

I was not able to do a proper review because there is a mix of tightly coupling and lously coupling. There are also todo's in the PR for some of the read state functions which make it hard to get a view on the effect of this feature on the pallet as a whole.

Nevertheless, your work has shown the difficulties of the pallet when not tightly coupling pallet assets and pallet balances. It means we have to benchmark the dispatchables ourselves but more importantly add logic for a secure implementation. For example, transfer called in the transfer dispatchable is not checking whether the origin is signed, we have to do this ourselves. In addition, this transfer function is never called in the transfer_keep_alive dispatchable that we were using before.

I think it will be easier to separate the two things you started to implement in this PR. This PR should add the native token functionality to the pallet without lously coupling pallet assets and balances. Ideally you rebase off of #150.

In a separate PR we refactor to lously couple the pallets.

@chungquantin
Copy link
Collaborator Author

chungquantin commented Aug 2, 2024

@Daanvdplas I am on my way to refactor to avoid the mix of lously couling and tightly coupling. However, AssetKind and AssetCriteria should not be removed because it does not bring the issue of lously coupling, mainly coming from type Assets. The another issue that is a blocker to me at the moment is how to create a shared type of the pallet_balances::Balance and pallet_assets::Balance because the BalanceOf<T> right now only supports the pallet_asset::Balance. I came over one post on Substrate stack exchange: https://substrate.stackexchange.com/questions/3604/convert-from-asset-balance-to-currency-balance

        // Two-way conversion between asset and currency balances
        type AssetToCurrencyBalance: Convert<Self::AssetBalance, BalanceOf<Self>>;
        type CurrencyToAssetBalance: Convert<BalanceOf<Self>, Self::AssetBalance>;

Which I think is a feasible way to handle the type conversion but it would create more type parameters to the config.

Edited

After playing around with type conversion, I think we should come with a loose coupling approach. There are shared types like amount: BalanceOf<T> which can't be solved without injecting the external interface, like UnionOf.

@peterwht
Copy link
Collaborator

peterwht commented Aug 2, 2024

Please see: #97 (comment)

@chungquantin chungquantin linked an issue Aug 4, 2024 that may be closed by this pull request
@chungquantin chungquantin self-assigned this Aug 4, 2024
@chungquantin chungquantin requested a review from Daanvdplas August 5, 2024 03:36
@Daanvdplas
Copy link
Collaborator

Daanvdplas commented Aug 7, 2024

Had a look at it again and want to put this here for later reference. There are 3 ways moving forward:

  1. Do nothing and have the devs use the ink native api:
    self.env().transfer
  • positive: _
  • negative: devs need to learn two apis to transfer tokens from the chain, potentially fungible docs will have to explain ink related functionality.
  1. Creating a separate API, excluding the AssetId parameter:
    fn transfer_native(to, amount)
  • positive: only provides functionality that devs can use, no need for devs to learn other apis than pop provides to transfer token from the chain.
  • negative: no value add from the api's ink provides, basically a wrapper, might become deprecated in the future, will only be one maybe two functions.
  1. Using the same API with AssetId == 0
    fn transfer(id, to, amount)
  • postive: in the ideal situation this is what we want, a single api, approve functionality for the native token.
  • negative: a lot of work, fork of pallet contracts to be able to use assets, store relay token in pallet assets and implement fees to be paid with assets. In addition, we have to throw a lot of errors because half of the functionality provided by fungibles use case can not be used for the dot token.

@Daanvdplas Daanvdplas force-pushed the daan/api branch 2 times, most recently from 5c36b4f to 6adf1b1 Compare August 16, 2024 14:26
@Daanvdplas Daanvdplas mentioned this pull request Aug 23, 2024
Base automatically changed from daan/api to main September 13, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat (pop api): add native token to local fungibles use case
4 participants