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

Dissociate accounts from tokens post run #2090

Merged
merged 8 commits into from
Jun 10, 2021
Merged

Conversation

Nana-EC
Copy link
Contributor

@Nana-EC Nana-EC commented Jun 8, 2021

Detailed description:
Many of the HTS acceptance tests associate the payer account with the newly created token.
Overtime this may build up and the account may hit the TOKENS_PER_ACCOUNT_LIMIT_EXCEEDED error.

Full @acceptance runs are also expensive and it might be worth being a bit more selective on tests included for every post deployment run

  • Dissociate accounts from tokens post run
  • Add @critical and @release flags for acceptance tests, refine tag distribution accordingly
  • Update README
  • Where possible use payer account as treasury as it avoids the cost of creating and funding a new treasury account
  • Reduce funding amounts for created accounts to cut costs
  • Remove camel case tag variants

Which issue(s) this PR fixes:
Fixes #2069

Special notes for your reviewer:
A full @acceptance run was costing about 150 ℏ, with these updates I was able to cut it down to 36 ℏ

@acceptance - total cost 31.6  ℏ
@release - total cost 19.2  ℏ
@critical -  total cost 6.5  ℏ

Checklist

  • Documentation added
  • Tests updated

Signed-off-by: Nana-EC <nana.essilfie-conduah@hedera.com>
@Nana-EC Nana-EC added bug Type: Something isn't working P3 test Test infrastructure, automated tests required, etc labels Jun 8, 2021
@Nana-EC Nana-EC added this to the Mirror 0.36.0 milestone Jun 8, 2021
@Nana-EC Nana-EC requested a review from a team June 8, 2021 06:07
@Nana-EC Nana-EC self-assigned this Jun 8, 2021
@Nana-EC Nana-EC added the process Build or test related tasks label Jun 8, 2021
Signed-off-by: Nana-EC <nana.essilfie-conduah@hedera.com>
@Nana-EC Nana-EC marked this pull request as ready for review June 8, 2021 06:12
@Nana-EC Nana-EC marked this pull request as draft June 8, 2021 14:55
Signed-off-by: Nana-EC <nana.essilfie-conduah@hedera.com>
Signed-off-by: Nana-EC <nana.essilfie-conduah@hedera.com>
@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #2090 (2f194bf) into master (126751e) will decrease coverage by 7.60%.
The diff coverage is 87.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2090      +/-   ##
============================================
- Coverage     87.05%   79.44%   -7.61%     
- Complexity     1744     1920     +176     
============================================
  Files           315      377      +62     
  Lines          7731     9304    +1573     
  Branches        740      879     +139     
============================================
+ Hits           6730     7392     +662     
- Misses          772     1649     +877     
- Partials        229      263      +34     
Impacted Files Coverage Δ
...ensus/ConsensusCreateTopicTransactionSupplier.java 0.00% <ø> (ø)
...sus/ConsensusSubmitMessageTransactionSupplier.java 0.00% <0.00%> (ø)
...ensus/ConsensusUpdateTopicTransactionSupplier.java 0.00% <ø> (ø)
...er/schedule/ScheduleCreateTransactionSupplier.java 0.00% <ø> (ø)
...supplier/token/TokenCreateTransactionSupplier.java 0.00% <ø> (ø)
...supplier/token/TokenUpdateTransactionSupplier.java 0.00% <ø> (ø)
...ra/mirror/monitor/config/MonitorConfiguration.java 0.00% <0.00%> (ø)
...edera/mirror/monitor/publish/PublishException.java 0.00% <0.00%> (ø)
.../hedera/mirror/monitor/publish/PublishMetrics.java 0.00% <0.00%> (ø)
...a/mirror/monitor/publish/TransactionPublisher.java 1.09% <0.00%> (-0.04%) ⬇️
... and 181 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfe92d7...2f194bf. Read the comment docs.

@Nana-EC Nana-EC requested a review from a team June 9, 2021 00:49
Signed-off-by: Nana-EC <nana.essilfie-conduah@hedera.com>
@Nana-EC Nana-EC requested a review from beeradb June 9, 2021 02:06
@Nana-EC Nana-EC marked this pull request as ready for review June 9, 2021 02:06
@Nana-EC Nana-EC requested a review from xin-hedera June 9, 2021 02:07
Copy link
Collaborator

@xin-hedera xin-hedera left a comment

Choose a reason for hiding this comment

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

just some minor issues

hedera-mirror-test/README.md Outdated Show resolved Hide resolved
hedera-mirror-test/README.md Outdated Show resolved Hide resolved
hedera-mirror-test/README.md Outdated Show resolved Hide resolved
hedera-mirror-test/README.md Outdated Show resolved Hide resolved
hedera-mirror-test/README.md Outdated Show resolved Hide resolved
hedera-mirror-test/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ijungmann ijungmann left a comment

Choose a reason for hiding this comment

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

Looks good, couple small things.

@ijungmann
Copy link
Contributor

Should we set the default Dockerfile command to use @critical or @release instead of @balancecheck?

@Nana-EC
Copy link
Contributor Author

Nana-EC commented Jun 9, 2021

Should we set the default Dockerfile command to use @critical or @release instead of @balancecheck?

Should be @critical thought I fixed that
Ah I see now. I'd say @critical makes sense as a balance check may not catch certain issues with the network

Signed-off-by: Nana-EC <nana.essilfie-conduah@hedera.com>
Signed-off-by: Nana-EC <nana.essilfie-conduah@hedera.com>
Signed-off-by: Nana-EC <nana.essilfie-conduah@hedera.com>
@Nana-EC Nana-EC requested a review from xin-hedera June 9, 2021 22:09
@sonarcloud
Copy link

sonarcloud bot commented Jun 9, 2021

Copy link
Collaborator

@xin-hedera xin-hedera left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

LGTM

@Nana-EC Nana-EC merged commit 9374a5b into master Jun 10, 2021
@Nana-EC Nana-EC deleted the acceptance-dissociate branch June 10, 2021 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Type: Something isn't working P3 process Build or test related tasks test Test infrastructure, automated tests required, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update HTS Acceptance Tests to Dissociate payer on complete
4 participants