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

Add Lockable Fractional Amounts of Fungible Tokens on Hedera #796

Merged

Conversation

Stephanie-Yi
Copy link
Contributor

Description:

Related issue(s):

Fixes #

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Stephanie Yi <ext_stephanie.yi@toko.network>
@netlify
Copy link

netlify bot commented Aug 30, 2023

Deploy Preview for hedera-hips ready!

Name Link
🔨 Latest commit fd8e4bd
🔍 Latest deploy log https://app.netlify.com/sites/hedera-hips/deploys/64ef9897d107590008f9be93
😎 Deploy Preview https://deploy-preview-796--hedera-hips.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Stephanie-Yi Stephanie-Yi force-pushed the add-lockable-fractional-amounts branch from f3f2292 to fd8e4bd Compare August 30, 2023 19:12
Signed-off-by: Stephanie Yi <ext_stephanie.yi@toko.network>
Copy link
Contributor

@sergmetelin sergmetelin left a comment

Choose a reason for hiding this comment

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

lgtm

@sergmetelin sergmetelin merged commit 75bfbfa into hashgraph:main Aug 30, 2023
6 of 7 checks passed
Copy link
Member

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

Very complicated HIP. The feature is sound and desirable but as usual - and here particularly - the devil is in the details.

{
token_id: 0.0.123456,
balance: [
{partition_id: 1, partition_id: 0.0.200001, balance: 10, locked: 5 },
Copy link
Member

Choose a reason for hiding this comment

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

partition_id here should be partition_number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be best to remove the first partition_id since the second partition_id: 0.0.200001 is identifiable enough for this partition.

partitionKey: 0x222abc..,
partitionMoveKey: 0x333ace…,
partitions: [
{partition_number:1, metadata: “ipfs://xxx”, partition_id: 0.0.200001},
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of partition_number? partition_id is a sufficient primary key. If partition_number is required how is it assigned (appears nowhere else in this document)?

- If she transfers tokens from partition-3, that transfer will return an appropriate error informing her that her tokens are locked.
- To make the transfer, she will use the simple form of the current crypto transfer as the existing tokenTransferList. If she wants to transfer tokens from partition-3, she (or the SDK) queries the mirror nodes or the wallets and maps partition-3 to the TokenID of `0.0.200003` and fills that in the token transfer list of the cryptoTransfer transaction.

## Specification
Copy link
Member

Choose a reason for hiding this comment

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

This proposed HIP has a lot of moving parts and though the wording seems clear I can’t help but think it would be more solid if it included some diagrams and/or pseudocode to make things especially concrete. There’s a lot of room for implementation fuzziness to sneak in to a big pile of words. At least if there were diagrams and/or pseudocode a contradiction between the two could be noticed and addressed - it might be either the words or the diagram/code - whereas as it stands things might slip though the implementation. And not just contradictions, but also holes.

In the same way, additional tables would be useful, e.g., which combination of parameters to APIs are valid and which are not? E.g., some of these APIs have 4 "buckets" on 2-dimensions: fungible vs non-fungible, and token-definition-id vs partition-id. It's not clear that all parameters of some of these APIs are acceptable to all 4 buckets. (And I'm not talking about the difference between "amount" and "serial#", that's clear enough.)


| Partition Key? | Partition-Move-Key? | Supply-Key? | Initial Supply? | Behavior |
| -------------- | ------------------- | ----------- | --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| N | Y | \* | \* | Acts like a normal token-def, no partition behavior |
Copy link
Member

Choose a reason for hiding this comment

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

Why is this line not N * * *?

IvanKavaldzhiev pushed a commit to LimeChain/hedera-improvement-proposal that referenced this pull request Nov 22, 2023
…ph#796)

Signed-off-by: Stephanie Yi <ext_stephanie.yi@toko.network>
Signed-off-by: IvanKavaldzhiev <ivankavaldzhiev@gmail.com>
kimbor pushed a commit to kimbor/hedera-improvement-proposal that referenced this pull request Jun 24, 2024
…ph#796)

Signed-off-by: Stephanie Yi <ext_stephanie.yi@toko.network>
Signed-off-by: Kim Rader <kim.rader@swirldslabs.com>
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.

3 participants