feat(erc20): implement ERC20Burnable extension #31

bidzyyys · 2024-04-03T11:44:51Z

Resolves #27

PR Checklist

Tests
Documentation

netlify · 2024-04-03T11:44:55Z

👷 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	`101af71`

codecov · 2024-04-04T14:51:54Z

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

Looks beautiful! Super clean and as expected 👍

The only changes we need before merging this are regarding Transfer events. Otherwise looks good!

alexfertel · 2024-04-05T10:02:33Z

contracts/src/erc20/mod.rs

+    /// [`Error::Overflow`] is returned.
+    /// If the `from` address doesn't have enough tokens, then the error
+    /// [`Error::InsufficientBalance`] is returned.
+    pub fn _update(


The reason why I didn't include this function in the base implementation is that it is pointless right now: we don't have a way to override internal methods.

I say we keep it for now.

Do you think there's a better name for it? I can't think of anything other than _transfer_impl and I don't like it.

Suggested change

pub fn _update(

pub fn _update(

I see your point why you had not included _update at first, but I introduced it to be used also in _burn mechanism. IMHO it is better to reuse as much code as possible instead of a need for changing the same in many places.

That is why _update is better IMHO

Right, but note that the above breaks when we need to optimize for performance as well.

fn _transfer( &mut self, from: Address, to: Address, value: U256, ) -> Result<(), Error> { if from == Address::ZERO { ... } if to == Address::ZERO { ... } self._update(from, to, value)?; Ok(()) } pub fn _update( &mut self, from: Address, to: Address, value: U256, ) -> Result<(), Error> { if from.is_zero() { ... } if to.is_zero() { ... } }

In the above, you can see that we are checking from.is_zero(), when we already checked that in _transfer. This is inefficient. _update improves code reusability, but it is also more costly to use.

This is only an example, because the only benefit of _update is not code reuse, but the flexibility it brings when extending the base ERC20 implementation, and that's why I think we should keep it.

Not including it in the base implementation was something totally on purpose: I was trying to strike a good balance between efficiency, code reuse, readability, etc. I still have my reservations about it, but let's see what the Stylus team does with respect to overriding internal methods.

I see your point, need to think how to solve it.

alexfertel · 2024-04-05T10:04:12Z

contracts/src/erc20/mod.rs

+    /// [`Error::Overflow`] is returned.
+    /// If the `from` address doesn't have enough tokens, then the error
+    /// [`Error::InsufficientBalance`] is returned.
+    pub fn _update(


We should say that it emits a Transfer event. And also add it to all the functions in this call stack.

And also stop emitting the event in transfer.

Good catch, thank you 👍

alexfertel · 2024-04-05T10:05:46Z

contracts/src/erc20/mod.rs

+        if from.is_zero() {
+            // Mint operation.
+            // Overflow check required:
+            // The rest of the code assumes that _total_supply never overflows


Suggested change

// The rest of the code assumes that _total_supply never overflows

// The rest of the code assumes that `_total_supply` never overflows.

alexfertel · 2024-04-05T10:08:36Z

contracts/src/erc20/mod.rs


        // Mint some tokens for Alice.
        let two = U256::from(2);
-        contract._balances.setter(alice).set(two);
+        contract._update(Address::ZERO, alice, two).unwrap();


This change makes the test now handle testing both transfer_from and _update. We should add separate tests for _update and keep other tests as isolated as possible. (Note we cannot avoid the actual call to _update that happens inside transfer_from.)

wdyt?

I see your point, it's valid. From my perspective, it is better to test if _update works as expected and than assume in tests that it works -> use _update.unwrap().
Touching every time the memory may be painful... Imagine a situation when due to some reasons _balances field gets renamed. After that change you would need to rename it in 1xxxxx places. While using _update you would need to adjust only _update function and the rest would work.

I am open to negotiation on that point @alexfertel @qalisander.

That's fair. We should add tests for _update() and that's it on my side 👍

(Note that I don't think we'll change _balances at all, but it's a valid point)

I can add tests to _update(), let's agree first how we should handle this (discussion in one of the above comments)

alexfertel · 2024-04-05T10:16:28Z

contracts/src/erc20/mod.rs

+    /// Indicates a failure where an overflow error in math occurred. Used in
+    /// `_update`.
+    Overflow(ERC20Overflow),


This is great thinking, though I am unsure of the implications. We should talk to the Solidity team about this and see what they think. I'll ask around.

Yes, and the rest of Rust teams. In general I am a fan of making it visible to developers and handling it as quick as possible instead of relying on built-in mechanisms that may change.

Small suggestion to name it ArithmeticOverflow? Feel free to ignore

I was expecting here some ideas 😅

alexfertel · 2024-04-05T10:24:27Z

contracts/src/erc20/extensions/burnable.rs

+
+sol_storage! {
+    pub struct ERC20Burnable {
+        ERC20 erc20;


The issue I see here is that we don't have C3 linearization yet in the SDK. This means that the inheritance may not end as you'd expect, i.e. in this graph, what does storage look like for the User's contract:

It may be good to research this and make sure all of this works: that's why I was trying to add examples, so that we can test these hierarchies. I'll try to find a solution ASAP when I have some time.

I think it is fine to merge this as is rn, but we need to keep in mind that this will probably not be it's final form.

Good point on that, I have some ideas for spikes but we need to have more than 1 extension to experiment 😅

qalisander

Looks good!
Extension structure may be will get changed in future but for now it should be fine

alexfertel

Beautiful!

I'm starting to see a couple of issues with the macros this way:

How do you control visibility? I see you used pub(crate), but what if I want my contract to be pub?
Imports are a bit awkward, though I think this is fine, since they won't be using an extension without using our contracts.
Exporting from the root is a bit weird as well, cause they'd be importing not from their corresponding modules but from the root.

Nonetheless, I think we should keep them and see what happens!

I left a few comments because there's a few clippy lints (we should try to fix those), but nothing complicated. After these are done this should be good to go! 🚀

alexfertel · 2024-04-09T18:23:48Z

contracts/src/erc20/extensions/burnable.rs

@@ -0,0 +1,192 @@
+//! Optional Burnable extension of the ERC-20 standard.
+
+#[macro_export]


Let's move this to just above macro_rules!. The rationale is:

Consistency with the way we declare the attributes in the rest of the crate.

This is preferred in Rust (check any item in std or any official crate like regex)

It's better to keep attributes close to what they decorate so that we don't have to jump around the code (also it's easy to miss them when they're like this).

alexfertel · 2024-04-09T18:26:05Z

contracts/src/erc20/extensions/burnable.rs

+/// This macro provides implementation of ERC-20 Burnable extension.
+///
+/// It adds `burn` and `burn_from` function
+/// to a custom token that contains `ERC20 erc20` attribute.


Suggested change

/// This macro provides implementation of ERC-20 Burnable extension.

///

/// It adds `burn` and `burn_from` function

/// to a custom token that contains `ERC20 erc20` attribute.

/// This macro provides an implementation of the ERC-20 Burnable extension.

///

/// It adds the `burn` and `burn_from` functions, and expects the token

/// to contain `ERC20 erc20` as a field. See [`crate::ERC20`].

alexfertel · 2024-04-09T18:27:03Z

contracts/src/erc20/extensions/burnable.rs

+/// Requires import of:
+/// * alloy_primitives::{Address, U256}


We should use fully qualified paths inside the macro instead of keeping this as a comment. I'll make the suggestions in the following comments.

alexfertel · 2024-04-09T18:28:49Z

contracts/src/erc20/extensions/burnable.rs

+        pub(crate) fn burn(&mut self, value: U256) -> Result<(), Error> {
+            self.erc20._burn(msg::sender(), value)
+        }


Suggested change

pub(crate) fn burn(&mut self, value: U256) -> Result<(), Error> {

self.erc20._burn(msg::sender(), value)

}

pub fn burn(&mut self, value: alloy_primitives::U256) -> Result<(), contracts::erc20::Error> {

self.erc20._burn(stylus_sdk::msg::sender(), value)

}

alexfertel · 2024-04-09T18:29:32Z

contracts/src/erc20/extensions/burnable.rs

+        pub(crate) fn burn_from(
+            &mut self,
+            account: Address,
+            value: U256,
+        ) -> Result<(), Error> {
+            self.erc20._spend_allowance(account, msg::sender(), value)?;
+            self.erc20._burn(account, value)
+        }


Suggested change

pub(crate) fn burn_from(

&mut self,

account: Address,

value: U256,

) -> Result<(), Error> {

self.erc20._spend_allowance(account, msg::sender(), value)?;

self.erc20._burn(account, value)

}

pub fn burn_from(

&mut self,

account: alloy_primitives::Address,

value: alloy_primitives::U256,

) -> Result<(), contracts::erc20::Error> {

self.erc20._spend_allowance(account, stylus_sdk::msg::sender(), value)?;

self.erc20._burn(account, value)

}

There is a problem with contracts::erc20::Error -> we cannot have it this way, while it breaks all imports in tests...

yeah, I feared that...not sure what the solution is 😢

Okay, fixed it!

pub(crate) fn burn_from( &mut self, account: alloy_primitives::Address, value: alloy_primitives::U256, ) -> 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()) }

alexfertel · 2024-04-09T18:30:58Z

contracts/src/erc20/mod.rs

+    /// # Events
+    ///
+    /// Emits a [`Transfer`] event.
+    pub fn _update(


Let's add a # Panics section to the docs since this function may panic now. (it's a clippy lint)

alexfertel · 2024-04-09T18:32:44Z

examples/erc20/src/lib.rs

@@ -21,6 +31,10 @@ sol_storage! {
 #[external]
 #[inherit(ERC20, Metadata)]
 impl Token {
+    // This macro implements ERC20Burnable functions -- `burn` and `burn_from`.
+    // Uses `erc20` Token's attribute.


Suggested change

// Uses `erc20` Token's attribute.

// Expects an `ERC20 erc20` as a field of `Token`.

examples/erc20/src/lib.rs

alexfertel

gogogogogo 🚀

bidzyyys linked an issue Apr 3, 2024 that may be closed by this pull request

ERC-20 Burnable #27

Closed

bidzyyys changed the title ~~ref(erc20): extract _update function (#27)~~ feat(erc20): implement Burnable extension Apr 3, 2024

bidzyyys self-assigned this Apr 3, 2024

bidzyyys added 4 commits April 4, 2024 15:53

ref(erc20): extract _update function (#27)

71df2a2

feat(erc20): implement Burnable extension (#27)

a4b80d4

test(erc20): fix tests for ERC20 after refactor

dc04ea3

test(erc20): tests for Burnable extension (#27)

af75199

bidzyyys force-pushed the feature/erc20-burnable branch from 388ec26 to af75199 Compare April 4, 2024 14:48

bidzyyys marked this pull request as ready for review April 4, 2024 14:49

bidzyyys requested review from alexfertel and qalisander April 4, 2024 14:49

bidzyyys added 2 commits April 4, 2024 16:53

ref(erc20): use nightly formatter

49c8d58

ref(erc20): use nightly formatter

19ffde4

bidzyyys changed the title ~~feat(erc20): implement Burnable extension~~ feat(erc20): implement ERC20Burnable extension Apr 4, 2024

alexfertel suggested changes Apr 5, 2024

View reviewed changes

bidzyyys and others added 6 commits April 5, 2024 16:40

ref(erc20): apply CR comments for Burnable

3557267

Merge branch 'main' into feature/erc20-burnable

4a1640e

test(erc20): tests for ERC20::_update

26b0243

ref(erc20): overflow should panic

1da8c70

ref(erc20): panic in _update

a586a3a

fix(grip): avoid leaking storage between tests

65f7a75

qalisander previously approved these changes Apr 9, 2024

View reviewed changes

Merge branch 'main' into feature/erc20-burnable

53c4aef

bidzyyys dismissed qalisander’s stale review via 53c4aef April 9, 2024 14:32

bidzyyys added 3 commits April 9, 2024 16:33

ref(erc20): ERC20Burnable as a macro (#27)

c4f2ed0

feat(erc20): add ERC20Burnable to example

43e4341

docs(erc20): improve docs for ERC20Burnable (#27)

907e6da

bidzyyys requested a review from alexfertel April 9, 2024 16:53

bidzyyys requested a review from qalisander April 9, 2024 16:53

alexfertel suggested changes Apr 9, 2024

View reviewed changes

bidzyyys added 2 commits April 10, 2024 09:13

ref(erc20): apply CR comments

a7d9835

lint(fmt): clippy

ad6488f

bidzyyys requested a review from alexfertel April 10, 2024 09:15

ref(erc20): apply CR comments

101af71

alexfertel approved these changes Apr 10, 2024

View reviewed changes

bidzyyys merged commit e578648 into main Apr 10, 2024
14 checks passed

bidzyyys deleted the feature/erc20-burnable branch April 10, 2024 09:45

	// The rest of the code assumes that _total_supply never overflows
	// The rest of the code assumes that `_total_supply` never overflows.

		@@ -0,0 +1,192 @@
		//! Optional Burnable extension of the ERC-20 standard.

		#[macro_export]

		/// Requires import of:
		/// * alloy_primitives::{Address, U256}

	// Uses `erc20` Token's attribute.
	// Expects an `ERC20 erc20` as a field of `Token`.

feat(erc20): implement ERC20Burnable extension #31

feat(erc20): implement ERC20Burnable extension #31

Conversation

bidzyyys commented Apr 3, 2024 • edited Loading

PR Checklist

netlify bot commented Apr 3, 2024 • edited Loading

👷 Deploy request for contracts-stylus pending review.

codecov bot commented Apr 4, 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

bidzyyys Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

alexfertel Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

bidzyyys Apr 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

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

Choose a reason for hiding this comment

alexfertel 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

alexfertel Apr 9, 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

alexfertel left a comment

Choose a reason for hiding this comment

bidzyyys commented Apr 3, 2024 •

edited

Loading

netlify bot commented Apr 3, 2024 •

edited

Loading

bidzyyys Apr 5, 2024 •

edited

Loading

alexfertel Apr 5, 2024 •

edited

Loading

bidzyyys Apr 8, 2024 •

edited

Loading

alexfertel left a comment •

edited

Loading

alexfertel Apr 9, 2024 •

edited

Loading