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

11129 assets burn mint #11353

Merged
merged 2 commits into from
Jul 12, 2023
Merged

11129 assets burn mint #11353

merged 2 commits into from
Jul 12, 2023

Conversation

endulab
Copy link
Contributor

@endulab endulab commented Jun 30, 2023

Burn and mint functionality for assets.

Adjust burning and minting flows to handle assets.
Supplies are passed from qml to nim as strings - "2" for ERC721, "1.5" for ERC20
String amounts are converted to Uint256 type. Additionally ERC20 amounts are converted to basic units (wei-like). Uint256 values are passed to backend functions and then coverted to strings which can be converted to bigInt.BigInt types.
Supply and RemainingSupply are exposed to qml as floats.

Issue #11129

It is a fully functional pr, but all operation on assets are integer-based, like with collectibles.

Please create a backup of your Status dir because the pr is doing migration.

@endulab endulab self-assigned this Jun 30, 2023
@status-im-auto
Copy link
Member

status-im-auto commented Jun 30, 2023

Jenkins Builds

Click to see older builds (35)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 386752b #1 2023-06-30 11:52:59 ~5 min tests/nim 📄log
✔️ 386752b #1 2023-06-30 11:53:15 ~5 min tests/imports 📄log
✔️ 386752b #1 2023-06-30 11:56:42 ~9 min macos/aarch64 🍎dmg
✔️ 386752b #1 2023-06-30 11:57:54 ~10 min macos/x86_64 🍎dmg
✔️ 386752b #1 2023-06-30 12:02:58 ~15 min linux/x86_64 📦tgz
✔️ 386752b #1 2023-06-30 12:12:18 ~24 min windows/x86_64 💿exe
✔️ 386752b #1 2023-06-30 12:31:11 ~43 min tests/e2e 📄log
✔️ d0811c6 #2 2023-07-06 07:57:19 ~5 min tests/imports 📄log
✔️ d0811c6 #2 2023-07-06 07:57:26 ~5 min macos/aarch64 🍎dmg
✔️ d0811c6 #2 2023-07-06 07:58:04 ~6 min tests/nim 📄log
✔️ d0811c6 #2 2023-07-06 08:01:32 ~9 min macos/x86_64 🍎dmg
✔️ d0811c6 #2 2023-07-06 08:09:22 ~17 min linux/x86_64 📦tgz
✔️ d0811c6 #2 2023-07-06 08:12:29 ~20 min windows/x86_64 💿exe
✖️ d0811c6 #2 2023-07-06 08:28:16 ~36 min tests/e2e 📄log
✔️ 73c6460 #3 2023-07-06 10:19:34 ~5 min tests/imports 📄log
✔️ 73c6460 #3 2023-07-06 10:19:42 ~5 min macos/aarch64 🍎dmg
✔️ 73c6460 #3 2023-07-06 10:20:20 ~6 min tests/nim 📄log
✔️ 73c6460 #3 2023-07-06 10:22:57 ~8 min macos/x86_64 🍎dmg
✔️ 73c6460 #3 2023-07-06 10:29:35 ~15 min linux/x86_64 📦tgz
✔️ 73c6460 #3 2023-07-06 10:36:37 ~22 min windows/x86_64 💿exe
✖️ 73c6460 #3 2023-07-06 10:58:35 ~44 min tests/e2e 📄log
✔️ 4dee2e6 #4 2023-07-06 16:09:37 ~5 min tests/imports 📄log
✔️ 4dee2e6 #4 2023-07-06 16:10:39 ~6 min tests/nim 📄log
✔️ 4dee2e6 #4 2023-07-06 16:12:11 ~7 min macos/aarch64 🍎dmg
✔️ 4dee2e6 #4 2023-07-06 16:12:58 ~8 min macos/x86_64 🍎dmg
✔️ 4dee2e6 #4 2023-07-06 16:20:05 ~15 min linux/x86_64 📦tgz
✖️ 4dee2e6 #4 2023-07-06 16:21:43 ~17 min tests/e2e 📄log
✔️ 4dee2e6 #4 2023-07-06 16:25:04 ~20 min windows/x86_64 💿exe
✔️ 5b18c36 #5 2023-07-07 09:19:48 ~5 min macos/aarch64 🍎dmg
✔️ 5b18c36 #5 2023-07-07 09:19:53 ~5 min tests/imports 📄log
✔️ 5b18c36 #5 2023-07-07 09:20:35 ~6 min tests/nim 📄log
✔️ 5b18c36 #5 2023-07-07 09:22:43 ~8 min macos/x86_64 🍎dmg
✔️ 5b18c36 #5 2023-07-07 09:26:43 ~12 min linux/x86_64 📦tgz
✔️ 5b18c36 #5 2023-07-07 09:34:00 ~19 min windows/x86_64 💿exe
✔️ 5b18c36 #5 2023-07-07 09:53:13 ~38 min tests/e2e 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 373898c #6 2023-07-07 10:21:24 ~4 min tests/imports 📄log
✔️ 373898c #6 2023-07-07 10:22:25 ~5 min macos/aarch64 🍎dmg
✔️ 373898c #6 2023-07-07 10:23:00 ~6 min tests/nim 📄log
✔️ 373898c #6 2023-07-07 10:29:50 ~12 min linux/x86_64 📦tgz
✔️ 373898c #6 2023-07-07 10:29:53 ~12 min macos/x86_64 🍎dmg
✔️ 373898c #6 2023-07-07 10:39:00 ~21 min windows/x86_64 💿exe
✔️ 373898c #6 2023-07-07 10:52:54 ~35 min tests/e2e 📄log
✔️ 520e4b3 #7 2023-07-12 08:47:53 ~4 min tests/imports 📄log
✔️ 520e4b3 #7 2023-07-12 08:48:21 ~5 min tests/nim 📄log
✔️ 520e4b3 #7 2023-07-12 08:49:52 ~6 min macos/aarch64 🍎dmg
✔️ 520e4b3 #7 2023-07-12 08:51:21 ~8 min macos/x86_64 🍎dmg
✔️ 520e4b3 #7 2023-07-12 08:56:50 ~13 min linux/x86_64 📦tgz
✔️ 520e4b3 #7 2023-07-12 09:03:38 ~20 min windows/x86_64 💿exe
✔️ 520e4b3 #7 2023-07-12 09:14:12 ~31 min tests/e2e 📄log

@endulab endulab linked an issue Jul 3, 2023 that may be closed by this pull request
Copy link
Contributor

@noeliaSD noeliaSD left a comment

Choose a reason for hiding this comment

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

LGTM! Not tested!

Added minor comments!

@@ -73,17 +74,17 @@ proc init*(self: Controller) =
proc deployContract*(self: Controller, communityId: string, addressFrom: string, password: string, deploymentParams: DeploymentParameters, tokenMetadata: CommunityTokensMetadataDto, chainId: int) =
self.communityTokensService.deployContract(communityId, addressFrom, password, deploymentParams, tokenMetadata, chainId)

proc airdropCollectibles*(self: Controller, communityId: string, password: string, collectiblesAndAmounts: seq[CommunityTokenAndAmount], walletAddresses: seq[string]) =
self.communityTokensService.airdropCollectibles(communityId, password, collectiblesAndAmounts, walletAddresses)
proc airdropTokens*(self: Controller, communityId: string, password: string, collectiblesAndAmounts: seq[CommunityTokenAndAmount], walletAddresses: seq[string]) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
proc airdropTokens*(self: Controller, communityId: string, password: string, collectiblesAndAmounts: seq[CommunityTokenAndAmount], walletAddresses: seq[string]) =
proc airdropTokens*(self: Controller, communityId: string, password: string, tokensAndAmounts: seq[CommunityTokenAndAmount], walletAddresses: seq[string]) =

proc airdropCollectibles*(self: Controller, communityId: string, password: string, collectiblesAndAmounts: seq[CommunityTokenAndAmount], walletAddresses: seq[string]) =
self.communityTokensService.airdropCollectibles(communityId, password, collectiblesAndAmounts, walletAddresses)
proc airdropTokens*(self: Controller, communityId: string, password: string, collectiblesAndAmounts: seq[CommunityTokenAndAmount], walletAddresses: seq[string]) =
self.communityTokensService.airdropTokens(communityId, password, collectiblesAndAmounts, walletAddresses)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.communityTokensService.airdropTokens(communityId, password, collectiblesAndAmounts, walletAddresses)
self.communityTokensService.airdropTokens(communityId, password, tokensAndAmounts, walletAddresses)

proc computeAirdropCollectiblesFee*(self: Controller, collectiblesAndAmounts: seq[CommunityTokenAndAmount], walletAddresses: seq[string]) =
self.communityTokensService.computeAirdropCollectiblesFee(collectiblesAndAmounts, walletAddresses)
proc computeAirdropFee*(self: Controller, collectiblesAndAmounts: seq[CommunityTokenAndAmount], walletAddresses: seq[string]) =
self.communityTokensService.computeAirdropFee(collectiblesAndAmounts, walletAddresses)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.communityTokensService.computeAirdropFee(collectiblesAndAmounts, walletAddresses)
self.communityTokensService.computeAirdropFee(tokensAndAmounts, walletAddresses)

Copy link
Member

@micieslak micieslak left a comment

Choose a reason for hiding this comment

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

It seems that #11376 should be taken into account.

I also wonder how qml side will handle uint256 types exposed from nim.

@endulab endulab force-pushed the 11129-assets-burn-mint branch 4 times, most recently from 4dee2e6 to 5b18c36 Compare July 7, 2023 09:14
@endulab endulab marked this pull request as ready for review July 7, 2023 09:14
@endulab endulab requested a review from jrainville July 7, 2023 09:14
@endulab
Copy link
Contributor Author

endulab commented Jul 7, 2023

According to discussion with @micieslak, the current state of this pr is fully functional, but all asset operations handle only integers. Floating points will be handled later.
I tested all flows for both types:

  • mint of asset & collectible
  • airdrop of asset and collectible
  • burn of asset and collectible
  • remote destruct of collectible

Also pr need to be rebased with #11439 in order to deploy contract with >1 supply.

Copy link
Member

@micieslak micieslak left a comment

Choose a reason for hiding this comment

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

Tested, works like a charm!

I did minting, airdrop and burn, everything look fine.
Screenshot from 2023-07-07 13-14-27

let tokenDto = self.controller.findContractByUniqueId(contractUniqueKey)
let amountStr = token["amount"].getFloat
Copy link
Member

Choose a reason for hiding this comment

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

it's float, not a string like the name suggests, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, right :)

else:
error "Converting amount - unknown token type", tokenType=tokenType

proc getTokenAndAmountList(self: Module, communityId: string, tokensJsonString: string): seq[CommunityTokenAndAmount] =
Copy link
Member

Choose a reason for hiding this comment

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

Whenever we take json as a string it would be nice to have short description what's the expected structure of that json :)

@endulab endulab requested a review from anastasiyaig July 7, 2023 11:20
@glitchminer
Copy link
Contributor

glitchminer commented Jul 10, 2023

Hey @endulab , seems like the majority of the actions are OK but sometimes are slow to be reflected in the app, or require a restart. Some further notes are below. Please let me know if you want them split out or if they will be addressed as part of this PR.

  1. OPEN Token appears to have 18 decimals on etherscan even though it was created to have only 2
    https://goerli.etherscan.io/token/0x8a5ec70974c88493bd2511e185b2d180ca185ac3
    image

  1. OPEN After initial minting was shown to be complete supply and account were not displaying expected values
    image

  1. OPEN After initial minting 1 airdrop button appeared enabled while the other was still disabled
    image

  1. OPEN Minted asset is included in activity as ETH
    Minted TEST and sent 1.5 via airdrop - it is included in wallet activity as 1.5 ETH and does not show up as a separate asset
    https://goerli.etherscan.io/token/0x8a5ec70974c88493bd2511e185b2d180ca185ac3
    image

  1. OPEN Minted asset hodler list remains empty after airdrop and restart
    image
    https://goerli.etherscan.io/token/0x8a5ec70974c88493bd2511e185b2d180ca185ac3

  1. OPEN Error during remote destruct not shown to user
    Attempted a second remote destruct before the first is completed. Tx failed but did not see corresponding toast / notice in app.
    https://goerli.etherscan.io/tx/0xaf2fec0e4a701feb97683bb4d50d31832bd84055dc08322ffc91184b10d263a1

  1. OPEN Burn and destruct pop-ups do not close after signing transaction
    This makes it difficult to click on toast link before it closes.
    image

@endulab
Copy link
Contributor Author

endulab commented Jul 11, 2023

@glitchminer thank you for great testing.

  1. Yes, under the hood, in blockchain, we always use decimals=18, exactly like for Eth. Decimal field used during minting is the setting for UI. It will have impact on how many digits after dot user can use/see. Possibility of using decimals will be added by @micieslak soon.
  2. There is a ticket for empty account - [Minting] Account name is empty during deployment #11219. No idea why remaining does not work. How do you have Total = 1,000 ? I had 1 during tests.
  3. We can't display holders for assets. There is a task for this.
  4. Display in the wallet should be discussed with the wallet team. Let's ping @dlipicar
  5. The same as 3.
  6. Hmm, It seems that we need to improve error handling. Can you please create a ticket for this?
  7. I also noticed it during burning and remote self-destruct. @noeliaSD can you check this?

Yes, let's address these issues in new tasks. This one is quite big and I want to merge it asap.

@dlipicar
Copy link
Contributor

dlipicar commented Jul 11, 2023

@endulab Since this token is "unknown" to us (as in, it's not in the wallet's list of supported tokens), it gets by default shown as "ETH". That's being changed here #11433 to prevent confusion, so unknown ERC20 tokens will now be handled specially.

What we actually need though is to make sure we add relevant tokens to the wallet's list. Something like, when you (attempt to) join a community that has deployed ERC20 contracts, you add such tokens to your supported tokens list. That way, balances, token name, transfers, etc will be displayed correctly across the app.

Copy link
Contributor

@glitchminer glitchminer left a comment

Choose a reason for hiding this comment

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

Approved. Any remaining issues to be raised and addressed separately.

@glitchminer
Copy link
Contributor

@endulab, thanks for the feedback! I've raised those now.

@noeliaSD , 1, 3, 8 are heading to you as discussed. Hope that's ok.

@dlipicar , do we need another issue opened for this point please make sure we add relevant tokens to the wallet's list. ?

Adjst burning and minting flows to handle assets.
Supplies are passed from qml to nim as strings - "2" for ERC721, "1.5" for ERC20
String amounts are converted to Uint256 type. Additionally ERC20 amounts are converted to basic units (wei-like, decimals=18).
Uint256 values are passed to backend functions and then coverted to strings which can be converted to bigInt.BigInt types.
Supply and RemainingSupply are exposed to qml as floats.

Issue #11129
@endulab endulab merged commit f829909 into master Jul 12, 2023
@endulab endulab deleted the 11129-assets-burn-mint branch July 12, 2023 09:16
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.

Assets airdrop & burn
8 participants