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

Update erc20 contract to 0.8 #68

Merged
merged 6 commits into from
May 25, 2020
Merged

Update erc20 contract to 0.8 #68

merged 6 commits into from
May 25, 2020

Conversation

webmaster128
Copy link
Contributor

@webmaster128 webmaster128 commented May 16, 2020

Based on #67

Part of #65

@webmaster128 webmaster128 mentioned this pull request May 16, 2020
4 tasks
@webmaster128 webmaster128 changed the title Update erc20 to 0.8 Update erc20 contract to 0.8 May 16, 2020
@webmaster128 webmaster128 added WIP work in progress ERC20 💰 labels May 16, 2020
@webmaster128 webmaster128 added this to Awaiting review in Blockchain Development via automation May 22, 2020
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good.

But if you are open, I can suggest a much more radical change using Uint128 for input types and making full use of cosmwasm storage. If you look at the staking demo contract, we have transfer and query balance implemented in many fewer lines. I don't know if it is too much magic for you, but I think this would make this contract much easier to extend.

messages: vec![],
log: vec![
log("action", "transfer_from"),
log(
"spender",
deps.api.human_address(&env.message.signer)?.as_str(),
deps.api.human_address(&env.message.sender)?.as_str(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We were discussing to remove the human / canonical address division if we use a binary encoding scheme, so we can just pass in canonical addresses.

This is one place where we will want to convert to human readable in any case. So we will need the hooks, just have to use them much less

@webmaster128
Copy link
Contributor Author

webmaster128 commented May 25, 2020

But if you are open, I can suggest a much more radical change using Uint128 for input types and making full use of cosmwasm storage.

Updated the public interfaces to Uint128 – good point. One helper function less we need. However, I kept the balance and allowance storage 16 raw bytes, which is more compact that using JSON encoded decimal strings. We can change that later if we want.

@ethanfrey
Copy link
Contributor

Fair enough, let's merge this!

@webmaster128 webmaster128 added merge when green See: https://github.com/phstc/probot-merge-when-green/ and removed WIP work in progress labels May 25, 2020
@webmaster128 webmaster128 merged commit 5ff9a31 into master May 25, 2020
Blockchain Development automation moved this from Awaiting review to Done May 25, 2020
@webmaster128 webmaster128 deleted the update-erc20-to-0.8 branch May 25, 2020 08:49
@ethanfrey ethanfrey removed this from Done in Blockchain Development Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ERC20 💰 merge when green See: https://github.com/phstc/probot-merge-when-green/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants