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

docs: Add test specifications for AccountAllowanceDeleteTransaction #280

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

Conversation

rwalworth
Copy link
Contributor

@rwalworth rwalworth commented Nov 19, 2024

Description:
This PR adds the test specifications for AccountAllowanceDeleteTransaction fields.

Related Issue(s):

Fixes #27

Notes for Reviewer(s):

This PR contains work in #279 so that should be reviewed and merged first.

Checklist

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

Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
…tion' into 00027-document-e2e-tests-accountallowancedeletetransaction
Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
@rwalworth rwalworth added the documentation Improvements or additions to documentation label Nov 19, 2024
@rwalworth rwalworth self-assigned this Nov 19, 2024
@rwalworth rwalworth linked an issue Nov 19, 2024 that may be closed by this pull request
@@ -0,0 +1,245 @@
# AccountAllowanceApproveTransaction - Test specification
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this file need to be in this PR? I believe it's already implemented in separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR contains #279 to get access to allowances.md which is used by AccountAllowanceDeleteTransaction. #279 should be reviewed and merged first.

Copy link
Contributor

@0xivanov 0xivanov left a comment

Choose a reason for hiding this comment

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

AccountAllowanceDeleteTransaction has several deprecated methods. Also in a lot of places you are saying approves and not deletes.
Overall this flow should be revisited.

Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
@rwalworth
Copy link
Contributor Author

AccountAllowanceDeleteTransaction has several deprecated methods. Also in a lot of places you are saying approves and not deletes. Overall this flow should be revisited.

I guess I got lazy with my copy-pasting! DeleteAllTokenNftAllowances should be the only function tested as it is the only function provided by this API. I removed the other functions.

There are deprecated methods but these only make modifications to local class variables that don't ever actually get sent to the network. I didn't think these were necessary to test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document E2E tests: AccountAllowanceDeleteTransaction
2 participants