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

Add traits and function implementations for missing extensions #14

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

Conversation

0xMMBD
Copy link

@0xMMBD 0xMMBD commented Dec 18, 2023

Added implementations for missing extensions.

Traits added:

  • PSP22Pausable
    • fn pause(&mut self)
    • fn unpause(&mut self)
  • PSP22Wrapper
    • fn deposit_for(&mut self, account: AccountId, amount: u128)
    • fn withdraw_to(&mut self, account: AccountId, amount: u128)
  • Ownable
    • fn owner(&self)
    • fn renounce_ownership(&mut self)
    • fn transfer_ownership(&mut self, new_owner: Option<AccountId>)

Traits modified:

  • PSP22
    • Added fn burn_from(&mut self, account: AccountId, value: u128)
  • PSP22Mintable
    • Changed mint to fn mint(&mut self, to: AccountId, value: u128)

Added error enum - OwnableError

Added function implementations for:

  • burn_from
  • deposit
  • withdraw

Example lib.rs implementation that contains all extensions enabled can be found here: https://gist.github.com/coredumped7893/38ea06963de8c5034b58a9267be50758

Code is based on this version of the psp22: https://github.com/Smart-Beaver/smart-contracts/tree/main/PSP22

Add function burn_from to PSP22 trait
Add "to" param to the mint function
Add .idea directory to the .gitignore file

Signed-off-by: Maciek Malik <[email protected]>
@coredumped7893
Copy link

@piotrswierzy

@piotrMocz piotrMocz requested a review from h4nsu December 18, 2023 11:18
Copy link
Member

@h4nsu h4nsu left a comment

Choose a reason for hiding this comment

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

Hi, thanks for contribution. However, I have some doubts about this PR:

  1. This repo is meant to provide a simple, minimal implementation of the standard. Traits Ownable and PSP22Pausable are not part of PSP22 standard. What is the point of adding these traits here, without implementation?
  2. I don't understand PSP22Wrapper trait, it seems to be just an alias for transfer and transfer_from. What is it useful for?Also, it introduces some chain related code (cross calling) to PSP22Data, which is supposed to be an internal data structure with token logic and should stay ink-and-chain agnostic.

I like the refinement of Mintable and Burnable to mimic the interfaces known from Ethereum. Ideally burn should be removed in favor of burn_from.

@0xMMBD
Copy link
Author

0xMMBD commented Dec 19, 2023

@h4nsu Regarding the second point. This is our attempt on migrating ERC20Wrapper to the ink. We wanted to keep the same functionality as openzeppelin has, so the comment from their repo gives us a summary what it does.

 * Users can deposit and withdraw "underlying tokens" and receive a matching number of "wrapped tokens". This is useful
 * in conjunction with other modules. For example, combining this wrapping mechanism with {ERC20Votes} will allow the
 * wrapping of an existing "basic" ERC-20 into a governance token.

That's why you can see some cross calling in the data.rs. Here we have E2E tests that contain comments for each step

@@ -267,4 +269,115 @@ impl PSP22Data {
value,
}])
}

/// Burns `value` tokens from `from` account.
pub fn burn_from(&mut self,
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of code in this function replicates fn burn - the only additional place here is the check and modification of allowances - so I'd change to code to reuse fn burn (i.e. call it and only add necessary allowances management around it).

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

Successfully merging this pull request may close these issues.

4 participants