-
Notifications
You must be signed in to change notification settings - Fork 78
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
11129 assets burn mint #11353
Conversation
Jenkins BuildsClick to see older builds (35)
|
There was a problem hiding this 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]) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.communityTokensService.computeAirdropFee(collectiblesAndAmounts, walletAddresses) | |
self.communityTokensService.computeAirdropFee(tokensAndAmounts, walletAddresses) |
There was a problem hiding this 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.
4dee2e6
to
5b18c36
Compare
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.
Also pr need to be rebased with #11439 in order to deploy contract with >1 supply. |
5b18c36
to
373898c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let tokenDto = self.controller.findContractByUniqueId(contractUniqueKey) | ||
let amountStr = token["amount"].getFloat |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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] = |
There was a problem hiding this comment.
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 :)
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.
|
@glitchminer thank you for great testing.
Yes, let's address these issues in new tasks. This one is quite big and I want to merge it asap. |
@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. |
There was a problem hiding this 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.
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
373898c
to
520e4b3
Compare
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.