feat(erc20): add Pausable and Capped extensions for ERC-20 token #49

bidzyyys · 2024-04-29T23:24:19Z

Closes #34 and #42.

PR Checklist

Tests
Documentation

netlify · 2024-04-29T23:25:02Z

👷 Deploy request for contracts-stylus pending review.

A Netlify team Owner will need to approve the deploy before you can run your build.

Are you a team Owner? Visit the deploys page to approve it →

Need more help? Learn more in the Netlify docs →

Name	Link
🔨 Latest commit	`fb6684b`

codecov · 2024-04-29T23:26:34Z

The author of this PR, bidzyyys, is not an activated member of this organization on Codecov.
Please activate this user on Codecov to display this PR comment.
Coverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations.
Please don't hesitate to email us at [email protected] with any questions.

alexfertel

First pass done! 🚀 Looking very nice. It's a super clean implementation.

I'll read the docs and come back with a few more thoughts!

I'll also push a couple of small commits polishing comments when I have the time.

alexfertel · 2024-04-30T12:41:47Z

Cargo.toml

  "lib/crypto",
  "lib/grip",
  "lib/grip-proc",
  "examples/erc20",
+  "proc-tests",


Why isn't this a file in contracts/tests/? I'm not sure this should be a member of the workspace, but instead just an integration test (or tests).

I had an issue with this setup - it was either cyclic dependency or sth with std or panic. Need to check that in incoming days.

Oh, yeah, ping me on this, it's really frustrating 😢 I think I've dealt with those issues so I may be helpful.

We can work on it in person soon ;)

alexfertel · 2024-04-30T12:46:35Z

Cargo.toml

@@ -1,11 +1,14 @@
 [workspace]
 members = [
  "contracts",
+  "erc20-proc",


I think this should be re-exported from contracts::erc20, and then erc20-proc should be a private member of the workspace (i.e., not published).

I like the idea.

alexfertel · 2024-04-30T12:52:44Z

contracts/src/erc20/extensions/burnable.rs

        ) -> Result<(), alloc::vec::Vec<u8>> {
-            self.erc20._spend_allowance(
-                account,
-                stylus_sdk::msg::sender(),
-                value,
-            )?;
-            self.erc20._burn(account, value).map_err(|e| e.into())
+            self.erc20.burn_from(account, value).map_err(|e| e.into())


This can be:

) -> Result<(), $crate::erc20::Error> { self.erc20.burn_from(account, value)

alexfertel · 2024-04-30T12:55:06Z

contracts/src/erc20/extensions/burnable.rs

    sol_storage! {
+        #[derive(Default)]


alexfertel · 2024-04-30T12:59:30Z

contracts/src/erc20/extensions/metadata.rs

@@ -8,6 +8,7 @@ pub const DEFAULT_DECIMALS: u8 = 18;

 sol_storage! {
    /// Optional metadata of the ERC-20 standard.
+    #[allow(clippy::pub_underscore_fields)]


Let's make this a crate-wide lint.

alexfertel · 2024-04-30T13:33:38Z

erc20-proc/src/lib.rs

+                 if from.is_zero() {
+                    let max_supply = self.cap();
+                    let supply = self.total_supply() + value;
+                    if supply > max_supply {
+                        return Err(contracts::erc20::Error::ERC20ExceededCap(
+                            contracts::utils::capped::ExceededCap {
+                                increasedSupply: supply,
+                                cap: max_supply,
+                            },
+                        ));
+                    }
+                }
+
+                // Call "wrapped" token
+                self.erc20._update(from, to, value)


Suggested change

if from.is_zero() {

let max_supply = self.cap();

let supply = self.total_supply() + value;

if supply > max_supply {

return Err(contracts::erc20::Error::ERC20ExceededCap(

contracts::utils::capped::ExceededCap {

increasedSupply: supply,

cap: max_supply,

},

));

}

}

// Call "wrapped" token

self.erc20._update(from, to, value)

self.erc20._update(from, to, value)

if from.is_zero() {

let max_supply = self.cap();

let supply = self.total_supply();

if supply > max_supply {

return Err(contracts::erc20::Error::ERC20ExceededCap(

contracts::utils::capped::ExceededCap {

increasedSupply: supply,

cap: max_supply,

},

));

}

}

alexfertel · 2024-04-30T13:37:18Z

examples/erc20/src/lib.rs

+    // Export ERC-20 features
+    erc20_impl!();
+
+    // Export ERC-20 Burnable features
    erc20_burnable_impl!();

+    // Export ERC-20 Capped features
+    erc20_capped_impl!();
+
+    // Export ERC-20 Pausable features
+    erc20_pausable_impl!();
+
+    // Add `mint` feature
+    fn mint(
+        &mut self,
+        account: Address,
+        value: U256,
+    ) -> Result<(), contracts::erc20::Error> {
+        if account.is_zero() {
+            return Err(contracts::erc20::Error::InvalidReceiver(
+                contracts::erc20::ERC20InvalidReceiver {
+                    receiver: Address::ZERO,
+                },
+            ));
+        }
+        self.erc20._update(Address::ZERO, account, value)
+    }


Yeah, this looks great!

alexfertel · 2024-04-30T13:37:50Z

proc-tests/Cargo.toml

+
+[dev-dependencies]
+contracts = { path = "../contracts", features = ["erc20_metadata", "erc20_burnable", "erc20_pausable", "erc20_capped"] }
+erc20-proc = { path = "../erc20-proc" }


Right, this dependency shouldn't be necessary, and instead, consumers should have access to these through contracts.

Good point.

alexfertel · 2024-04-30T13:40:36Z

proc-tests/src/erc20/mod.rs

Yeah, these are great. Though they should run on a fork, until we have one that works, this is good enough.

Can you expand on your idea with fork pls

Ah, so we should make some of these more advanced checks e2e by deploying the contract to a local fork and communicating with it. We should try to keep unit tests simple and move most of the actual contract behavior checks to integration tests.

This is not a strong opinion though, and until we have a proper local fork that we can use, I think it's fine we do our testing like this.

local fork of the test node you mean?

Ideally, Arbitrum One once the Stylus upgrade is done. For now,https://stylus-testnet.arbitrum.io/rpc.

alexfertel · 2024-04-30T13:41:49Z

proc-tests/src/lib.rs

+#[cfg(test)]
+mod erc20;


yeah, this should be contracts/tests/macros.rs and shouldn't need a cfg flag (haven't tested).

qalisander

Extremely great job so far!
As I pointed before, storage abstraction is a cool approach.
And the derive macro for contracts implementation is promising.
Awesome POC!

The drawbacks as I can see is that code inside of proc macro has linear complexity O(n) from creation of new logic. Like for erc721 and for every other extension new macro with logic should be created :( And maintaining, changing and I suppose security audit of them will be a bit cumbersome.

qalisander · 2024-04-30T14:04:20Z

contracts/src/erc20/extensions/burnable.rs

@@ -28,9 +37,7 @@ macro_rules! erc20_burnable_impl {
            &mut self,
            value: alloy_primitives::U256,
        ) -> Result<(), alloc::vec::Vec<u8>> {


Is it possible to have it as a typed error?

Yes! See #49 (comment)

Nice catch!

qalisander · 2024-04-30T14:15:49Z

erc20-proc/src/lib.rs

+
+    let expanded = quote! {
+        impl contracts::erc20::IERC20Virtual for #name {
+            fn _update(


I can see that IERC20Virtual should have a bit more methods. Seems we should duplicate all of them and wrap call to super object. Otherwise it is pointless for the trait to be Virtual. Since just a single method _update inside can be virtual.

p.s. I think it is a bit of a flaw in the design that leads to code duplication.

qalisander · 2024-04-30T14:21:21Z

erc20-proc/src/lib.rs

+
+    let expanded = quote! {
+        impl contracts::erc20::IERC20Virtual for #name {
+            fn _update(


Here is the same as in the previous comment. Shouldn't you wrap all methods inside IERC20Virtual? Other (parent) contract extension can override different method and here you just call base implementation (auto implement within rust trait) instead of forwarding the other (overriden) method to the super contract.

qalisander · 2024-04-30T14:23:23Z

erc20-proc/src/lib.rs

+
+    let expanded = quote! {
+        impl contracts::erc20::IERC20Virtual for #name {
+            fn _update(


Same. Other methods will be silently auto implemented with base

qalisander · 2024-04-30T14:26:50Z

examples/erc20/src/lib.rs

 }

 #[external]
-#[inherit(ERC20, Metadata)]
+#[inherit(Metadata)]


Basically for POC every #[inherit] would be better to be removed. Since you are trying to bypass stylus sdk limitation for inheritance that exist currently.
I would vote to have this solution not relying on stylus inheritance at all.
May be we would skimp on WASM compiled a bit :D
Wdt?

qalisander · 2024-04-30T14:39:53Z

examples/erc20/src/lib.rs

+    erc20_pausable_impl!();
+
+    // Add `mint` feature
+    fn mint(


As I can see this is a new method exposed externally.
Method with exactly same name that exists inside of the preceding macros can not be added (rust name collision should occur).
Stylus sdk's #[selector(...)] macro can help it or the method can be overridden..
Curious how can look like from the end user perspective single overriden function from e.g. IERC20Virtual?

But this method (mint) is not a part of ERC20.

It is not. I'm saying it is the new one. How user can override existing method?

qalisander · 2024-04-30T14:42:19Z

erc20-proc/Cargo.toml

@@ -0,0 +1,16 @@
+[package]
+name = "erc20-proc"


How rational can it be to have a dedicated proc crate for every contract? Since we're planning to use contracts crate for every contract

Good point, I need to improve the structure.

qalisander · 2024-04-30T14:47:34Z

erc20-proc/src/lib.rs

+        }
+    };
+
+    TokenStream::from(expanded)


into can be used

qalisander · 2024-04-30T14:48:40Z

proc-tests/src/erc20/mod.rs

Can you expand on your idea with fork pls

qalisander · 2024-04-30T15:20:07Z

erc20-proc/src/lib.rs

+                value:  alloy_primitives::U256,
+            ) -> Result<(), contracts::erc20::Error> {
+                // Call "wrapped" token
+                self.erc20._update(from, to, value)


Also every macro assumes that erc20 field exists inside. Weak typing, a bit fragile. @alexfertel what do you think how it can be improved?

Not sure yet.

I can try to refactor it to achieve sth like this, but not sure if it is possible.

#[derive(IERC20)] #[inner_field(name = "erc20")] struct ERC20Wrapper { erc20: ERC20, }

qalisander · 2024-05-08T19:06:46Z

contracts/src/erc20/mod.rs

+impl IERC20Virtual for ERC20 {}
+
+/// Default implementation of `IERC20` trait for `ERC20`.
+#[external]


Basically you don't need this #[external] and many other "externals". They're just auto implementing Router trait and needed if there is an #[inherit(..)] macro

qalisander · 2024-05-08T19:28:18Z

examples/erc20/src/lib.rs

-    // This macro implements ERC20Burnable functions -- `burn` and `burn_from`.
-    // Expects an `ERC20 erc20` as a field of `Token`.
+    // Export ERC-20 features
+    erc20_impl!();


One concern I found recently. Seems critical. This is the code of expanded #[external] macro:

I can not see any methods from _impl!(...) macro in the router trait implementation. Seems like TokenStream at #[external] macros encounters tokens like "_impl!(...)". And nothing is exported. Looks like a function macros has less priority in this case.

argh, that's big bummer. We would have found this earlier if we had tests....

Basically I'm working on tests. It was matter of time to find it

But yep. Kinda priorities should be making working and compiling code first, then polishing documentation and codestyle

alexfertel · 2024-05-15T16:07:16Z

@bidzyyys, per our discussion during the retreat, should we close this one for now, and if we decide to revisit this approach, we can reopen the PR?

bidzyyys · 2024-05-15T17:57:44Z

@bidzyyys, per our discussion during the retreat, should we close this one for now, and if we decide to revisit this approach, we can reopen the PR?

Yes, we need to close it, but let's keep the branch for the future.

bidzyyys added 20 commits April 10, 2024 11:50

feat(erc20): add Pausable contract (#34)

f070ade

ref(erc20): wip, move code to traits

272751e

ref(erc20): wip, move pausable interface into trait

89be81c

feat(erc20): implement Pausable extension as wrapper over ERC20 (#34)

80074cf

ref(erc20): Burnable as a Trait

1fe5c44

ref(erc20): wip traits and wrappers

37a5842

ref(erc20): ERC20 as trait and impl

9337ffd

ref(erc20): bring back ERC20Burnable

7efadbf

ref(erc20): restructure code into procedural macros

f59dd69

ref(erc20): ERC20 Burnable and Pausable example

109eb6f

feat(erc20): add ERC20Capped extension (#42)

3749781

feat(erc20): export interfaces as macros

bb1552a

test(erc20): add tests for macros

5ecdf90

ref(erc20): use IERC20Storage for tests

9bbac2d

fix: infinite loop in IERC20Storage::_set_allowance

a646cc5

docs(erc20): update docs for ERC-20 base token

86122f3

docs(erc20): docs for ERC-20 Pausable extension

3072bc9

docs(erc20): update docs for ERC-20 Capped extension

335c7b4

Merge branch 'main' into bidzyyys/extensions

b640935

docs(erc20): update docs for procedural macros

13b9de9

bidzyyys requested review from alexfertel and qalisander April 29, 2024 23:24

bidzyyys self-assigned this Apr 29, 2024

This was linked to issues Apr 29, 2024

ERC-20 Pausable #34

Closed

ERC-20 Capped #42

Closed

bidzyyys added this to the Release v0.1.0 milestone Apr 29, 2024

bidzyyys added 2 commits April 30, 2024 12:27

fix: remove examples from procedural macros

9f90ec9

docs(erc20): update docs for ERC-20 Burnable extension

7f8459b

bidzyyys force-pushed the bidzyyys/extensions branch from c99e569 to 9a40a2e Compare April 30, 2024 11:10

fix: clippy linting

fb6684b

bidzyyys force-pushed the bidzyyys/extensions branch from 9a40a2e to fb6684b Compare April 30, 2024 11:15

alexfertel suggested changes Apr 30, 2024

View reviewed changes

qalisander reviewed Apr 30, 2024

View reviewed changes

qalisander reviewed May 8, 2024

View reviewed changes

bidzyyys closed this May 15, 2024

alexfertel removed this from the Release v0.1.0 milestone Jun 1, 2024

alexfertel deleted the bidzyyys/extensions branch June 21, 2024 16:49

		#[cfg(test)]
		mod erc20;

feat(erc20): add Pausable and Capped extensions for ERC-20 token #49

feat(erc20): add Pausable and Capped extensions for ERC-20 token #49

Conversation

bidzyyys commented Apr 29, 2024 • edited Loading

PR Checklist

netlify bot commented Apr 29, 2024 • edited Loading

👷 Deploy request for contracts-stylus pending review.

codecov bot commented Apr 29, 2024

alexfertel left a 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

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

qalisander left a comment • edited Loading

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

qalisander Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qalisander Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

qalisander Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

bidzyyys May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qalisander May 7, 2024 • edited Loading

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

qalisander May 8, 2024 • edited Loading

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

alexfertel commented May 15, 2024

bidzyyys commented May 15, 2024

bidzyyys commented Apr 29, 2024 •

edited

Loading

netlify bot commented Apr 29, 2024 •

edited

Loading

qalisander left a comment •

edited

Loading

qalisander Apr 30, 2024 •

edited

Loading

qalisander Apr 30, 2024 •

edited

Loading

qalisander Apr 30, 2024 •

edited

Loading

bidzyyys May 6, 2024 •

edited

Loading

qalisander May 7, 2024 •

edited

Loading

qalisander May 8, 2024 •

edited

Loading