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

redesign initial distribution logic #78

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

0xChqrles
Copy link
Contributor

@0xChqrles 0xChqrles commented Dec 19, 2023

Resolves #77.

Also brings some modifications to the _enforce_holders_limit internal. The method can also decrease holders count if the sender has no tokens left after the transfer.

Lastly, it might be interesting to discuss about adding additional checks in the _enforce_holders_limit such as asserting the amount param or recipient are not null. With current ERC20 implementation, those checks are useless, but it makes this method rely on ERC20 behaviour. Without those checks, this method is a kind of ERC20 extension, which is not a bad design, but also not necessarily what we want.

Copy link

vercel bot commented Dec 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unruggable-memecoin ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2023 3:27pm

);
// assert max holders limit is not reached
assert(
current_holders_count < MAX_HOLDERS_BEFORE_LAUNCH,
Copy link
Contributor

Choose a reason for hiding this comment

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

Souldn't it be current_holders_count <= MAX_HOLDERS_BEFORE_LAUNCH?`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

current_holders_count is not increase yet, so it's < but we can also check that current_holders_count + 1 <= MAX_HOLDERS_BEFORE_LAUNCH

@0xEniotna
Copy link
Contributor

0xEniotna commented Dec 19, 2023

Can you cnahge this:

fn transferFrom(
            ref self: ContractState,
            sender: ContractAddress,
            recipient: ContractAddress,
            amount: u256
        ) -> bool {
            let caller = get_caller_address();
            self.erc20._spend_allowance(sender, caller, amount);
            self._transfer(sender, recipient, amount);
            true
        }

into this:

fn transferFrom(
            ref self: ContractState,
            sender: ContractAddress,
            recipient: ContractAddress,
            amount: u256
        ) -> bool {
            return self.transfer_from(sender, recipient, amount);
        }

(not related to your PR at all but since it's not merged yet...)

Copy link
Contributor

@0xEniotna 0xEniotna left a comment

Choose a reason for hiding this comment

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

LGTM ! I hope we won't have to change every single tests once again in the future...

@AbdelStark AbdelStark merged commit 98d8bf5 into keep-starknet-strange:main Dec 19, 2023
3 checks passed
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.

[contracts] Initial tokens distribution redesign
3 participants