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

PSP22 chain extension example #1244

Merged
merged 24 commits into from
Sep 8, 2022
Merged

Conversation

stiiifff
Copy link
Contributor

@stiiifff stiiifff commented May 3, 2022

Example implementation of the PSP22 Fungible Token Standard as a chain extension, supporting a multi-token system provided by the FRAME assets pallet.
It demonstrates how ink! contracts (L2) can interact with native assets (L1) from the chain runtime in a standardised way.

@stiiifff stiiifff added the A-examples [examples] Work item label May 3, 2022
@xgreenx
Copy link
Collaborator

xgreenx commented May 3, 2022

It is bad that we did double work=( We could cooperate together to deliver but it is too late.
We did the same in the OpenBrush.

But it is still good to have several implementations=)

We plan to create a production-ready chains extension and define its interface in the PSP. So if you have some though regarding our implementation it will be cool to hear them here or in PSP=)

I want to highlight that maybe some contract wants to transfer his tokens, so it is why we added the Origin enum into the chain extension where the developer can specify it(Origin::Caller or Origin::Self).

But we also realized the problem with Origin::Caller. Allowing anyone to transfer/approve tokens on behalf of the caller without any restriction creates a vulnerability. Any contract can be malicious and can steal all tokens. I think you need to highlight that by commenting on the implementation of transfer in the chain extension.

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented May 3, 2022

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the examples/* contracts from this branch with cargo-contract 1.5.0-ef06f4d and comparing them to ink! master:

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator 1.00 K
adder 2.04 K
contract-terminate 0.92 K 275_000
contract-transfer 8.36 K 75_000
delegate-calls 2.89 K 76_242
delegator 6.34 K 232_138
dns 8.81 K 225_000
erc1155 17.15 K 450_000
erc20 8.42 K 225_000
erc721 11.62 K 600_000
flipper 1.24 K 75_000
forward-calls 2.87 K 151_412
incrementer 1.14 K
mother 12.24 K
multisig 25.79 K 470_483
payment-channel 7.95 K
psp22-extension +7.16 K 7.16 K
rand-extension 3.79 K 75_000
set-code-hash 1.49 K
subber 2.06 K
trait-erc20 8.69 K 225_000
trait-flipper 0.97 K 75_000
trait-incrementer 1.12 K 150_000
updated-incrementer 9.78 K
upgradeable-flipper 1.48 K

Link to the run | Last update: Tue Aug 23 14:08:39 CEST 2022

@codecov-commenter
Copy link

codecov-commenter commented May 3, 2022

Codecov Report

Merging #1244 (5dad725) into master (2b7f20e) will decrease coverage by 0.40%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
- Coverage   70.52%   70.11%   -0.41%     
==========================================
  Files         189      190       +1     
  Lines        5910     5940      +30     
==========================================
- Hits         4168     4165       -3     
- Misses       1742     1775      +33     
Impacted Files Coverage Δ
crates/metadata/src/layout/mod.rs 73.10% <0.00%> (-1.69%) ⬇️
crates/lang/ir/src/ir/attrs.rs 81.99% <0.00%> (-0.28%) ⬇️
crates/allocator/src/bump.rs 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stiiifff
Copy link
Contributor Author

stiiifff commented May 3, 2022

@xgreenx Thanks for your feedback ! I was unaware of your implementation. This is just a simple, incomplete example. My motivation was to show a more advanced chain extension example than the existing FetchRandomExtension one, and I thought that PSP22 was a good use case. So I don't see it as double work.
Good point re. the caller in the transfer function.

@Maar-io
Copy link

Maar-io commented May 30, 2022

hi @stiiifff
Do you still plan to add this as example?
I'd like to use it for one of my workshops

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

I'm really sorry for taking so long to look at this! Thanks for your time though, I like the addition of this 🙇

examples/psp22-extension/Cargo.toml Outdated Show resolved Hide resolved
examples/psp22-extension/README.md Outdated Show resolved Hide resolved
examples/psp22-extension/README.md Outdated Show resolved Hide resolved
examples/psp22-extension/README.md Show resolved Hide resolved
@HCastano
Copy link
Contributor

HCastano commented Aug 2, 2022

@bidzyyys Hey, are you going to address the open comments?

@Wiezzel
Copy link
Contributor

Wiezzel commented Aug 10, 2022

@HCastano I will resolve the comments.

@Wiezzel Wiezzel self-assigned this Aug 10, 2022
@Wiezzel Wiezzel requested a review from a team as a code owner August 22, 2022 15:14
@Robbepop Robbepop removed their request for review August 28, 2022 09:28
Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Thanks for updating this @Wiezzel!

Can you fork the substrate-contracts-node, add this chain extension to it, and share the repo? I want to see if this code compiles. We'll need to think of a way to check with our CI in the future

examples/psp22-extension/Cargo.toml Outdated Show resolved Hide resolved
examples/psp22-extension/lib.rs Outdated Show resolved Hide resolved
examples/psp22-extension/lib.rs Outdated Show resolved Hide resolved
examples/psp22-extension/lib.rs Outdated Show resolved Hide resolved
pub value: Balance,
}

pub struct Psp22Extension;
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't compile due to some changes with how chain extensions work. You can read more about the breaking changes here: paritytech/substrate#11874.

There's also these two PRs which you might find useful/interesting

examples/psp22-extension/README.md Outdated Show resolved Hide resolved
examples/psp22-extension/lib.rs Show resolved Hide resolved
Wiezzel and others added 2 commits August 31, 2022 09:41
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
@Wiezzel
Copy link
Contributor

Wiezzel commented Sep 5, 2022

@HCastano I've made some changes according to your comments.
Here's the substarte-contratcs-node code with the extension installed: https://github.com/Wiezzel/substrate-contracts-node/pull/1
CI tests are failing, but it seems to me that is not related to any changes from this PR. I've run cargo test -p ink_lang --test compile_tests on master and it fails as well.

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Here's the substarte-contratcs-node code with the extension installed: https://github.com/Wiezzel/substrate-contracts-node/pull/1

Thanks for that!

CI tests are failing, but it seems to me that is not related to any changes from this PR. I've run cargo test -p ink_lang --test compile_tests on master and it fails as well.

Can you merge master and see if that fixes your issues?

@Wiezzel
Copy link
Contributor

Wiezzel commented Sep 7, 2022

@HCastano All checks passed! 😄

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Couple of small things left but we're almost here 💪

Comment on lines 406 to 407
// FIXME: This is a bit of a shortcut. It was made because the documentation
// for Mutate::approve does not specify the result of subsequent calls.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't leave FIXMEs (or TODOs) in the code. File an issue and we can address it in a follow up PR

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I'll open an issue as soon as this PR is merged.

.gitlab-ci.yml Outdated Show resolved Hide resolved
Wiezzel and others added 2 commits September 8, 2022 10:51
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this!

@HCastano HCastano merged commit a314b34 into use-ink:master Sep 8, 2022
@stiiifff stiiifff deleted the psp22-chainext-example branch September 11, 2022 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-examples [examples] Work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants