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

feat: track hbar expenses for submit tx #2705

Closed
wants to merge 5 commits into from

Conversation

georgi-l95
Copy link
Member

@georgi-l95 georgi-l95 commented Jul 16, 2024

Description:
This PR modifies submit transaction logic to account for hbar expenses. Previously we didn't track operator expenses for ethereum transactions. This PR is using executeGetTransactionRecord method, which takes the record and deduct the cost from the budget.

Related issue(s):

Fixes #2703

Notes for reviewer:

Checklist

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

Signed-off-by: georgi-l95 <glazarov95@gmail.com>
@georgi-l95 georgi-l95 added the bug Something isn't working label Jul 16, 2024
@georgi-l95 georgi-l95 added this to the 0.52.0 milestone Jul 16, 2024
@georgi-l95 georgi-l95 self-assigned this Jul 16, 2024
@georgi-l95 georgi-l95 linked an issue Jul 16, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Jul 16, 2024

Acceptance Tests

  20 files  259 suites   39m 28s ⏱️
608 tests 587 ✔️ 4 💤 17
837 runs  812 ✔️ 8 💤 17

Results for commit 48648bc.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 16, 2024

Tests

    2 files  161 suites   12s ⏱️
866 tests 865 ✔️ 1 💤 0
879 runs  878 ✔️ 1 💤 0

Results for commit 48648bc.

♻️ This comment has been updated with latest results.

Signed-off-by: georgi-l95 <glazarov95@gmail.com>
Signed-off-by: georgi-l95 <glazarov95@gmail.com>
@georgi-l95 georgi-l95 marked this pull request as ready for review July 16, 2024 13:54
Copy link
Contributor

@ebadiere ebadiere left a comment

Choose a reason for hiding this comment

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

LG, but can we add tests?

Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Some suggestions

}
const transactionRecord = await new TransactionRecordQuery()
.setTransactionId(resp.transactionId!)
.setValidateReceiptStatus(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing the nodeIds selection.
Was this intentionally left out?
I believe the optimization allows for you to get it from the node you submitted to for shorter latency.

if (shouldLimit) {
throw predefined.HBAR_RATE_LIMIT_EXCEEDED;
}
const transactionRecord = await new TransactionRecordQuery()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use executeQuery() as it will handle metric and hbar rate limit

packages/relay/src/lib/clients/sdkClient.ts Show resolved Hide resolved
Signed-off-by: georgi-l95 <glazarov95@gmail.com>
Signed-off-by: georgi-l95 <glazarov95@gmail.com>
Copy link

@quiet-node quiet-node modified the milestones: 0.52.0, 0.53.0 Jul 19, 2024
@georgi-l95
Copy link
Member Author

Replaced by #2714

@georgi-l95 georgi-l95 closed this Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand Hbar Limiter coverage
5 participants