-
Notifications
You must be signed in to change notification settings - Fork 88
chore: refactor token examples #378
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
chore: refactor token examples #378
Conversation
exploreriii
left a comment
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.
Hi, this is going great! You've mostly achieved all that was requested, would you like to add a little more detail to the key steps make it easier to understand and perhaps add some checks if useful?
|
@exploreriii I have done the requirements, if there is anything I should change please let me know |
exploreriii
left a comment
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.
Thank you for all this
Could I ask you in the final functions to add a step by step?
e.g.
def token_associate...
"""
- Create an account that will associate tokens
- Create the tokens to associate to the account
- Associate the tokens to the account
- Verify the tokens are both associated to the account
It is not necessary but will help to improve the clarity for users
Also maybe some additional checks could be useful but not necessary, mentioned above
exploreriii
left a comment
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.
Hi. The output is looking great, thorough and easy to follow.
However, some of the code can be tidied up further see some ideas below
examples/token_dissociate.py
Outdated
| """ | ||
| Create two new tokens (one NFT and one fungible) for demonstration purposes. | ||
| - This example shows how to associate and dissociate both types of tokens (NFT and fungible) |
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.
keep this briefer please so its easier to read
E.g
This example shows how to dissociate NFT and fungible tokens from an account that already has them associated.
We demonstrate an end-to-end example that first creates the tokens, the account, associates them and finally, dissociates.
|
Hi @MonaaEid just checking in on this |
|
Hi @exploreriii I will finish this asap |
|
Looking great, let me know when its ready to review. Thank you |
|
Good morning @exploreriii , could you please review the updates |
exploreriii
left a comment
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.
Hi @MonaaEid . This is generally looking really great. I've taken a first pass through these recent changes - I need to look at it in more detail tomorrow or soon.
Meanwhile I've added a couple of ideas if you think they are worth pushing forward
|
please let me know when this is ready to review again |
|
Hello@exploreriii , I will. |
|
No problem, thanks for the update. Don't want to leave you hanging in case you were expecting a review. |
|
Hi @MonaaEid we have had a lot of PR's relating to examples but I have tried to keep the ones you are working on untouched. I think I created an issue on topic_create which conflits with yours, sorry. |
|
@exploreriii could you please review? |
|
Yes, thank you! If any other issue interest you in the meantime, please feel free to take it on while I finish reviewing this |
examples/token_mint_non_fungible.py
Outdated
| """Generate a new supply key for the token.""" | ||
| print("\nSTEP 1: Generating a new supply key...") | ||
| supply_key = PrivateKey.generate("ed25519") | ||
| supply_key = PrivateKey.generate_ed25519() |
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.
could be nice if we'd have generate() and then in there an env var either setting it to ED25519 or ECDSA
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.
Is this ok?
supply_key = PrivateKey.generate(os.getenv('HSDK_KEY_TYPE', 'ed25519'))
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.
yes, just maybe just KEY_TYPE. but else perfect:)
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.
would be great if we could implement this across the board
exploreriii
left a comment
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.
@MonaaEid thank you SO MUCH for this!
I see you added queries to verify, plus improved the existing scripts quite a bit
Please rebase then let's follow up with Nadine to see any final changes
|
I'm just noting the solo checks didn't run. Hopefully they re-trigger on rebasing |
|
Need you to rebase, your changelog and examples/topic_create.py have conflicts. try then read merge_conflicts.md! |
|
I'm not familiar on the key generation issues, but did see other sdks start to recommend to not use ed25519 |
exploreriii
left a comment
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.
Sorry @MonaaEid you have two commits (near the start) that are not marked as verified, see the commits tab please.
You can pick at these commits and edit to sign them
else you can try to soft revert and re-sign
|
@MonaaEid : Hi there, are you still working on this issue? If so, do you require help with the signing of commits? Thanks |
|
@nadineloepfe, I am working on it right now. and I need to know what to do. could you please help? |
|
This will involve picking at those specific commits and then rebasing, which is quite difficult, also because you have a lot of merges from main you'll see find the commit hash of your unsigned commits Change each pick to edit for the commits you want to sign: You can alos soft revert your 40 commits, then re commit (in fewer commits) and sign appropriately: git reset soft--HEAD ~40, git commit -s -S -M "....." You'll also need to rebase. useful: Get in touch via discord.md and i can help you if you want |
|
Hi @MonaaEid would you like me to do the final steps for you? |
It would be nice. thank you. |
8685cd0 to
c53f383
Compare
|
Hi @MonaaEid I'll be releasing a new python sdk release soon - |
2473279 to
7ebfad1
Compare
7ebfad1 to
478cd57
Compare
b0b0fea to
3015f47
Compare
3015f47 to
89b1391
Compare
Description:
This PR refactors the following example scripts to improve readability, modularity, and developer experience:
token_delete.py
token_dissociate.py
token_freeze.py
token_unfreeze.py
token_mint_fungible.py
token_mint_non_fungible.py
topic_create.py
Related issue(s):
Fixes #370
Checklist