Add ERC20Burnable preset #252

koloz193 · 2022-04-01T15:10:49Z

Created an erc20 burnable preset from the mocked file and removed the mock since it's no longer needed.

Fixes #237

Depends on:

andrew-fleming · 2022-04-19T00:40:48Z

Hey @koloz193, thank you for submitting this PR! I know you're aware that we're considering some changes to the library (#267 and you mentioned in your comment^). Once we get this straightened out, we'll be able to review :)

koloz193 · 2022-05-03T13:27:54Z

@andrew-fleming makes sense. i ended up doing a fresh refactor based on the current state of main

andrew-fleming · 2022-05-04T06:22:47Z

@koloz193 ahh yeah, the rebasing conflicts must have looked like a nightmare

koloz193 · 2022-05-04T11:20:41Z

@koloz193 ahh yeah, the rebasing conflicts must have looked like a nightmare

yup, once i saw like 10 different files i quickly decided against trying to attempt the rebase

andrew-fleming · 2022-05-24T04:51:58Z

@koloz193 Ownable is merged! I appreciate your patience, sir 🙏 at your leisure, would you mind updating your branch?

koloz193 · 2022-05-24T18:39:25Z

awesome! will merge in main and clean it up!

andrew-fleming

This looks great, sir! Thanks again for doing this :) I left a couple of comments

andrew-fleming · 2022-05-25T18:59:03Z

src/openzeppelin/token/erc20/ERC20_Burnable.cairo

+@view
+func owner{
+        syscall_ptr: felt*,
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr


I think we should move this up and keep it grouped with the Getters and maybe add an Externals comment to demarcate the @external methods for consistency

andrew-fleming · 2022-05-25T22:04:42Z

tests/token/erc20/test_ERC20_Burnable.py

+    new_balance = sub_uint(INIT_SUPPLY, AMOUNT)
+
+    execution_info = await erc20.balanceOf(account1.contract_address).invoke()
+    assert execution_info.result.balance == new_balance


We should check that account2's allowance is correctly subtracted here. Just to be thorough, I think we should include additional tests for:

burnFrom emits Transfer event

burnFrom reverts when burn amount exceeds balance

burnFrom reverts when account is the zero address (I believe spender as the zero address is indirectly covered with "insufficient allowance")

good call, ill add these tests in

Thank you, sir! I'm re-reviewing the enumerable PR as well, so hopefully we can get your contributions in soon!

@andrew-fleming added testing for the event. looks like burnFrom reverts when burn amount exceeds balance is covered by the following test and having account as the zero address results in an underflow error since the zero address cant give any approval

Ah my mistake. Thank you for adding in the event test!

andrew-fleming

@koloz193 This looks excellent and good to go for me! Thank you so much :)

koloz193 · 2022-07-13T15:10:16Z

@andrew-fleming @martriay any update on this 😄

martriay · 2022-07-13T17:00:08Z

This is currently on our project roadmap, just that it's prioritized as "Low". This means we will tackle it whenever we finish higher priority issues, or we reprioritize based on new information. Sorry it's taking so long 🙇.

martriay

@koloz193 sorry for taking so long! I updated the PR to match the latest directory structure (#350). I left a few comments but otherwise good to go!

martriay · 2022-07-29T19:09:17Z

src/openzeppelin/token/erc20/presets/ERC20Burnable.cairo

    ):
    ERC20.initializer(name, symbol, decimals)
    ERC20._mint(recipient, initial_supply)
+    Ownable.initializer(owner)


why is it ownable?

good call, removing ownable

martriay · 2022-07-29T19:09:35Z

src/openzeppelin/token/erc20/presets/ERC20Burnable.cairo

+@external
+func transferOwnership{
+        syscall_ptr: felt*,
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr
+    }(newOwner: felt):
+    Ownable.transfer_ownership(newOwner)
+    return ()
+end
+
+@external
+func renounceOwnership{
+        syscall_ptr: felt*,
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr
+    }():
+    Ownable.renounce_ownership()


I'm not sure I get why are these needed

martriay · 2022-07-29T19:15:42Z

tests/token/erc20/test_ERC20Burnable.py

+    assert_event_emitted(
+        tx_exec_info,
+        from_address=erc20.contract_address,
+        name='Transfer',
+        data=[
+            account1.contract_address,
+            ZERO_ADDRESS,
+            *AMOUNT
+        ]
+    )


what do you think about splitting this into two different tests? that'd be consistent with burn whose storage effects and emitted events are tested separately

yea easy enough

martriay · 2022-08-05T15:50:44Z

Hey @koloz193! Do you think you will be able to finish this one off or should we pick it up? Thanks :)

koloz193 · 2022-08-05T15:57:14Z

@martriay yup I'll wrap it up this weekend!

martriay · 2022-08-15T08:15:15Z

@koloz193 kind ping (:

koloz193 · 2022-08-16T14:09:11Z

@martriay just pushed the changes, sorry for the delay but ran the tests and it looks good

martriay

Looking good! Requested small changes, plus resolving merge conflicts I think we're good! Thanks again :)

martriay · 2022-08-17T22:05:55Z

src/openzeppelin/token/erc20/presets/ERC20Burnable.cairo

@@ -1,11 +1,12 @@
 # SPDX-License-Identifier: MIT
+# OpenZeppelin Cairo Contracts v0.2.1 (token/erc20/presets/ERC20Burnable.cairo)


Suggested change

# OpenZeppelin Cairo Contracts v0.2.1 (token/erc20/presets/ERC20Burnable.cairo)

# OpenZeppelin Cairo Contracts v0.3.1 (token/erc20/presets/ERC20Burnable.cairo)

I think we need to update this

martriay · 2022-08-17T22:07:56Z

src/openzeppelin/token/erc20/presets/ERC20Burnable.cairo

@@ -19,7 +20,8 @@ func constructor{
        symbol: felt,
        decimals: felt,
        initial_supply: Uint256,
-        recipient: felt
+        recipient: felt,
+        owner: felt


Suggested change

owner: felt

there's no owner anymore

martriay · 2022-08-17T22:08:17Z

tests/token/erc20/test_ERC20Burnable.py

@@ -42,29 +46,32 @@ async def erc20_init(contract_classes):
            DECIMALS,
            *INIT_SUPPLY,
            account1.contract_address,        # recipient
+            account1.contract_address,        # owner


Suggested change

account1.contract_address, # owner

martriay · 2022-08-18T17:16:51Z

Amazing! Now there's only the merge conflicts left.

koloz193 · 2022-08-22T14:04:32Z

Amazing! Now there's only the merge conflicts left.

not anymore 😄

added erc20 burnable preset

80a904c

koloz193 force-pushed the feature/erc_20_burnable branch from ddf927e to 80a904c Compare May 3, 2022 13:26

koloz193 added 3 commits May 25, 2022 09:41

Merge branch 'main' into feature/erc_20_burnable

a1cc66c

updated burnable with new ownable module

815244b

removed unused import and changed constructor to initializer

f72dcb0

andrew-fleming reviewed May 26, 2022

View reviewed changes

koloz193 added 3 commits June 21, 2022 09:56

Merge branch 'main' into feature/erc_20_burnable

cf74af8

added event testing

3c03786

Merge branch 'main' into feature/erc_20_burnable

db492c6

koloz193 requested a review from andrew-fleming June 24, 2022 19:32

andrew-fleming approved these changes Jun 25, 2022

View reviewed changes

martriay added 3 commits July 29, 2022 15:47

Merge branch 'main' into feature/erc_20_burnable

3c9f386

adapt to new dir structure

f262032

remove unused mock

d1e4bfb

martriay reviewed Jul 29, 2022

View reviewed changes

martriay changed the title ~~ERC20_Burnable preset~~ Add ERC20Burnable preset Jul 29, 2022

removed ownable and split up burn from test

eb3a36f

koloz193 requested a review from martriay August 16, 2022 14:09

martriay suggested changes Aug 17, 2022

View reviewed changes

removed lingering owner and bumped version comment

b055e1f

koloz193 requested a review from martriay August 18, 2022 13:47

addressed merge conflicts

b180773

martriay approved these changes Aug 25, 2022

View reviewed changes

martriay merged commit 258a14f into OpenZeppelin:main Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ERC20Burnable preset #252

Add ERC20Burnable preset #252

koloz193 commented Apr 1, 2022 •

edited

Loading

andrew-fleming commented Apr 19, 2022

koloz193 commented May 3, 2022

andrew-fleming commented May 4, 2022

koloz193 commented May 4, 2022

andrew-fleming commented May 24, 2022

koloz193 commented May 24, 2022

andrew-fleming left a comment

andrew-fleming May 25, 2022

andrew-fleming May 25, 2022

koloz193 Jun 2, 2022

andrew-fleming Jun 2, 2022

koloz193 Jun 21, 2022

andrew-fleming Jun 25, 2022

andrew-fleming left a comment

koloz193 commented Jul 13, 2022

martriay commented Jul 13, 2022

martriay left a comment •

edited

Loading

martriay Jul 29, 2022

koloz193 Aug 16, 2022

martriay Jul 29, 2022

martriay Jul 29, 2022

koloz193 Aug 16, 2022

martriay commented Aug 5, 2022

koloz193 commented Aug 5, 2022

martriay commented Aug 15, 2022

koloz193 commented Aug 16, 2022

martriay left a comment

martriay Aug 17, 2022

martriay Aug 17, 2022

martriay Aug 17, 2022

martriay commented Aug 18, 2022

koloz193 commented Aug 22, 2022

		@@ -1,11 +1,12 @@
		# SPDX-License-Identifier: MIT
		# OpenZeppelin Cairo Contracts v0.2.1 (token/erc20/presets/ERC20Burnable.cairo)

	# OpenZeppelin Cairo Contracts v0.2.1 (token/erc20/presets/ERC20Burnable.cairo)
	# OpenZeppelin Cairo Contracts v0.3.1 (token/erc20/presets/ERC20Burnable.cairo)

Add ERC20Burnable preset #252

Add ERC20Burnable preset #252

Conversation

koloz193 commented Apr 1, 2022 • edited Loading

andrew-fleming commented Apr 19, 2022

koloz193 commented May 3, 2022

andrew-fleming commented May 4, 2022

koloz193 commented May 4, 2022

andrew-fleming commented May 24, 2022

koloz193 commented May 24, 2022

andrew-fleming 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

andrew-fleming left a comment

Choose a reason for hiding this comment

koloz193 commented Jul 13, 2022

martriay commented Jul 13, 2022

martriay 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

Choose a reason for hiding this comment

martriay commented Aug 5, 2022

koloz193 commented Aug 5, 2022

martriay commented Aug 15, 2022

koloz193 commented Aug 16, 2022

martriay 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

martriay commented Aug 18, 2022

koloz193 commented Aug 22, 2022

koloz193 commented Apr 1, 2022 •

edited

Loading

martriay left a comment •

edited

Loading