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

Issue #5: Adding in TokenUnfreezeTransaction #54

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

aceppaluni
Copy link

This PR aims to fix issue #5 as it adds functionality for token unfreeze transactions.

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

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

Hi @aceppaluni. Lots of good stuff. Nearly there - key areas for improvement is testing.

@exploreriii
Copy link
Contributor

Thanks. Please rebase to ensure no conflicts with main, I believe the readme file has now split in two, with a new one in examples.

@nadineloepfe nadineloepfe requested a review from exploreriii March 4, 2025 08:44
@exploreriii
Copy link
Contributor

def unfreeze_token(client, token_id, account_id, freeze_key):
"""Unfreeze the specified token with the given account."""
transaction = TokenUnfreezeTransaction(account_id=receipt_id, token_id=token_id_1)
Double check this section if the values are passed correctly, your test may not be runnning as intended, I think

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

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

Hello, the solo test has failed due to a small issue. Please have another go and let me know if you need help to set up solo on your own github actions. Additionally, once succesfull, you might have to squash the commits to keep with the requirements.

passing account_id here will not work without first defining account_id in the test.py file.
You have a few options with what you can use from those that have been created, refer back to the start of the file, I think you can use operator, recipient or even new_account_id.

In terms of what is best, I think recipient_id (which is an account id named like that). That's because recipient_id has already had their token frozen in main(). It would be a good test to see if that same frozen token account can become unfrozen.

if you are passing on unfreeze_token(client, token_id_1, recipient_id, freeze_key), make sure to accept these parameters in the same order (name can change) and then use the names you chose. If its confusing which names are being used, can try to pass them as per the main and don't change the names.

def unfreeze_token(client, token_id_1, account_id, freeze_key):
"""Unfreeze the specified token with the given account."""
transaction = TokenUnfreezeTransaction(account_id=receipt_id <-- (typo with the n & where is recipient?), token_id=token_id_1)

transaction.freeze_with(client)
transaction.sign(client.operator_private_key)
transaction.sign(freeze_key)

test.py Outdated
Comment on lines 193 to 195
def unfreeze_token(client, token_id, account_id, freeze_key):
"""Unfreeze the specified token with the given account."""
transaction = TokenUnfreezeTransaction(account_id=receipt_id, token_id=token_id_1)
Copy link
Contributor

Choose a reason for hiding this comment

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

def unfreeze_token(client, token_id, account_id, freeze_key):
"""Unfreeze the specified token with the given account."""
transaction = TokenUnfreezeTransaction(account_id=receipt_id, token_id=token_id_1)

The passed and used values should match

@@ -342,6 +361,7 @@ def main():
associate_token(client, recipient_id, recipient_private_key, [token_id_1, token_id_2])
transfer_token(client, operator_id, operator_key, recipient_id, token_id_1)
freeze_token(client, token_id_1, recipient_id, freeze_key)
unfreeze_token(client, token_id_1, account_id, freeze_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, the solo test has failed due to a small issue.

passing account_id here will not work without first defining account_id in the test.py file.
You have a few options with what you can use from those that have been created, refer back to the start of the file, I think you can use operator, recipient or even new_account_id.

In terms of what is best, I think recipient_id (which is an account id named like that). That's because recipient_id has already had their token frozen in main(). It would be a good test to see if that same frozen token account can become unfrozen.

if you are passing on unfreeze_token(client, token_id_1, recipient_id, freeze_key), make sure to accept these parameters in the same order (name can change) and then use the names you chose. If its confusing which names are being used, can try to pass them as per the main and don't change the names.

def unfreeze_token(client, token_id_1, account_id, freeze_key):
"""Unfreeze the specified token with the given account."""
transaction = TokenUnfreezeTransaction(account_id=receipt_id <-- (typo with the n & where is recipient?), token_id=token_id_1)

transaction.freeze_with(client)
transaction.sign(client.operator_private_key)
transaction.sign(freeze_key)

Copy link
Author

Choose a reason for hiding this comment

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

Am I changing this back?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I had a look at the prior version, which was also nearly there, please correct me if I missed anything. Not quite changing it back.

The prior:
In test.py, you were not testing token unfreeze because you did not call it in the main.
Key issue 1: absence of unfreeze_token(client, token_id_1, recipient_id, freeze_key) in main. While you had a def unfreeze_token(client, token_id, account_id, freeze_key), it was not called in the main. Therefore, the unfreeze test never runs.

Note: if you are calling unfreeze_token in the main, you'll need to ensure the parameters are received correctly in def unfreeze_token. Your approach of def unfreeze_token(client, token_id, account_id, freeze_key) could be fine with unfreeze_token(client, token_id_1, recipient_id, freeze_key). While you are using account_id when you passed recipient_id, what matters is the sequence and you are keeping it. Therefore, this bit is fine as before but it may make more sense to call account_id as recipient_id though artistic preference.

In current test.py:
Key issue 1: you are passing consistent versions for token unfreeze, which is good, but the parameter account_id is not defined for the main to use.

In main you have: unfreeze_token(client, token_id_1, account_id, freeze_key)
Your function is: def unfreeze_token(client, token_id_1, account_id, freeze_key).
but where is account_id created?

Currently, test.py does not have an account_id, it has an operator id and a recipient id.
So while account_id could be nice to have, the file doesn't know what it is. One option is to create account_id. Another option is to use operator id or recipient id.

Key issue 2: to unfreeze a token, I believe you'll need to have a frozen token first. Therefore, ensure you are unfreezing from the same account that has theirs frozen. In main, the currently frozen account is recipient_id.

Summary:
You can do
def unfreeze_token(client, token_id, recipient_id, freeze_key) with unfreeze_token(client, token_id_1, recipient_id, freeze_key)
or
def unfreeze_token(client, token_id, account_id, freeze_key) (as before) with unfreeze_token(client, token_id_1, recipient_id, freeze_key) (new)

You cannot do:
unfreeze_token(client, token_id_1, account_id, freeze_key)
def unfreeze_token(client, token_id_1, account_id, freeze_key)
without a defined account_id that was previously frozen, I believe.

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.

2 participants