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

Initial implementation #1

Merged
merged 8 commits into from
Sep 29, 2023
Merged

Initial implementation #1

merged 8 commits into from
Sep 29, 2023

Conversation

h4nsu
Copy link
Member

@h4nsu h4nsu commented Sep 26, 2023

First draft of pure ink! implementation of PSP22 standard.

This is a double-purpose crate:

  1. can be used as a dependency in some other contract to allow for type-safe cross-contract calls using provided PSP22 trait (via contract_ref! macro)
  2. can be compiled into a ready to use PSP22 contract with:
    cargo contract build --release --features "contract"

lib.rs Outdated Show resolved Hide resolved
types.rs Outdated Show resolved Hide resolved
lib.rs Outdated
}
let to_balance = self.balance_of(to);
// Total supply is limited by u128.MAX so no overflow is possible
self.data.balances.insert(to, &(to_balance + value));

Choose a reason for hiding this comment

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

Not sure if relevant, but take a look at this use-ink/ink#1831 regarding overflow checks, because I think there are some subtleties

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed it looks like soon all unchecked arithmetic is going to be banned from ink! contracts. I replaced all occurrences of + and - (which were impossible to overflow "by design") with saturating_add and saturating_sub.

lib.rs Show resolved Hide resolved
lib.rs Outdated Show resolved Hide resolved
Comment on lines +191 to +195
if amount == 0 {
self.data.allowances.remove((owner, spender));
} else {
self.data.allowances.insert((owner, spender), &amount);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether it wouldn't be slightly better to do it like:

  1. self.data.allowances.remove((owner, spender)) in line L186 - remove returns "previous" value.
  2. Check if new_allowance (amount) equals 0:
  • if "yes", do nothing
  • if "no", call insert.

Seems like that would be 1 access to the Mapping less.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, it could be a little bit more efficient.

Currently there are 3 branches this code can end up with and the resulting Mapping operations are:

  1. allowance < by : 1 get
  2. allowance = by : 1 get + 1 remove
  3. allowance > by : 1 get + 1 insert

In the version you propose that would be:

  1. allowance < by : 1 remove + 1 insert
  2. allowance = by : 1 remove
  3. allowance > by : 1 remove + 1 insert

Not sure what is the relative cost of remove vs get, but the total number of operations globally is the same. Obviously, we expect variant 2 to "fire" much more often than variant 1, so expected number of operations is lower in your version. But I feel like this is a miniscule gain that sacrifices quite a lot of readability in a code that is supposed to be shown to community, so I'd lean towards simplicity > efficiency in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

In the version you propose that would be:

  1. allowance < by : 1 remove + 1 insert
  2. allowance = by : 1 remove
  3. allowance > by : 1 remove + 1 insert
  1. current_allowance < by : 1 remove (no insert since we would fail early with InsufficientAllowance
  2. current_allowance = by : 1 remove
  3. current_allowance > by: 1 remove + 1 insert

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we're not 100% sure Mapping.take() is stable, lets stay with the simpler 100% safe implementation for now

Copy link
Contributor

@deuszx deuszx left a comment

Choose a reason for hiding this comment

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

There's one thing that I'm missing in Rust (that I had in Scala) that is ability to write tests for an abstraction and have it automatically run for all of its implementation...

Here I'm thinking about a contract (punt NOT intended) of the PSP22 standard that all of its implementations should adhere to. That would be a set of rules that the implementation has to follow. Stuff like (from the top of my head):

  1. transferring more than allowance results in XYZ error
  2. increase_allowance(by_n) + increase_allowance(by_m) === increase_allowance(by_n + by_m)
  3. PSP22::new(m) creates an instance of a token with all of its balance in one account - m's.
  4. Event emittion
  5. Error returned
  6. etc.

I can see it done in two ways:

  1. "chamsko" - meaning there is a set of unit/e2e tests written for Token here.
  2. not so "chamsko" - written as a macro that can be invoked on any struct implementing PSP22 trait. In which case its implementors can add the suite via some macro psp22_standard_tests!(my_psp22_impl) and have all the tests ran against their impl to check for adhering with the standard.

What do you think?

types.rs Show resolved Hide resolved
@h4nsu
Copy link
Member Author

h4nsu commented Sep 29, 2023

@deuszx I like the generic tests idea, but I want to make a separate PR with tests.

I updated this PR with changes based on your suggestions. I decided to add one more thing to the logic - zero value operations result in no-ops (ie transfer of 0 tokens does not generate a Transfer event)

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.

3 participants