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

Fix generated transaction IDs are not unique issue. #1587

Merged
merged 2 commits into from
Sep 11, 2023
Merged

Conversation

thenswan
Copy link
Contributor

@thenswan thenswan commented Sep 4, 2023

Description:
Fix generated transaction IDs are not unique issue.

Related issue(s):
Fixes #1557

Checklist

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

@thenswan thenswan requested a review from rokn September 4, 2023 16:14
@thenswan thenswan requested a review from SimiHunjan as a code owner September 4, 2023 16:14
@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Patch coverage: 85.71% and project coverage change: +0.08% 🎉

Comparison is base (26d4188) 82.11% compared to head (9bd55aa) 82.20%.
Report is 13 commits behind head on develop.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #1587      +/-   ##
=============================================
+ Coverage      82.11%   82.20%   +0.08%     
- Complexity      3569     3585      +16     
=============================================
  Files            186      186              
  Lines          11675    11692      +17     
  Branches        1146     1148       +2     
=============================================
+ Hits            9587     9611      +24     
+ Misses          1611     1603       -8     
- Partials         477      478       +1     
Files Changed Coverage Δ
...n/java/com/hedera/hashgraph/sdk/TransactionId.java 69.60% <85.71%> (+0.43%) ⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 104 to 110
do {
currentTime = System.currentTimeMillis() * 1_000_000L;
lastTime = monotonicTime.get();
if (currentTime <= lastTime) currentTime = lastTime + 1000L;
}

while (!monotonicTime.compareAndSet(lastTime, currentTime));
Copy link

Choose a reason for hiding this comment

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

Can we add a comment here and maybe extract the consts for more code clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rokn please check it now :)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

90.9% 90.9% Coverage
0.0% 0.0% Duplication

@thenswan thenswan merged commit 412332c into develop Sep 11, 2023
@thenswan thenswan deleted the issue-1557 branch September 11, 2023 14:21
@@ -64,6 +63,13 @@ public final class TransactionId implements Comparable<TransactionId> {
@Nullable
private Integer nonce = null;

private static final long MICROSECONDS_PER_MILLISECOND = 1_000_000L;

Choose a reason for hiding this comment

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

The name is not right, should be NANOSECONDS_PER_MILLISECOND.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check #1605

@@ -64,6 +63,13 @@ public final class TransactionId implements Comparable<TransactionId> {
@Nullable
private Integer nonce = null;

private static final long MICROSECONDS_PER_MILLISECOND = 1_000_000L;

private static final long CLOCK_DRIFT_THRESHOLD = 1_000L;

Choose a reason for hiding this comment

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

It's not clock drift but rather minimal timestamp increment in nanoseconds. Note that it allows for up to 1M TPS, which is assumed plenty for one client process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check #1605

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.

Generated transaction IDs are not unique
3 participants