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.

Sign up for GitHub

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

Jump to bottom

Implement eth_sendRawTransaction for large Ethereum TXs #281

Merged
merged 10 commits into from
Jul 6, 2022
Merged

Implement eth_sendRawTransaction for large Ethereum TXs #281

merged 10 commits into from
Jul 6, 2022

Conversation

natanasow
Copy link
Collaborator

@natanasow natanasow commented Jun 29, 2022 •
edited by Nana-EC
Loading

Signed-off-by: nikolay [email protected]

Description:
Support eth_sendRawTransaction for large Ethereum TXs

Related issue(s):

Fixes #15

Notes for reviewer:

Checklist

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

@natanasow natanasow marked this pull request as draft June 29, 2022 12:30
@natanasow
Copy link
Collaborator Author

This should work on-the-fly but there is something broken and still debugging it.

Nana-EC
Nana-EC reviewed Jun 30, 2022
View reviewed changes
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.

Looking good.
Early comments

packages/relay/src/lib/clients/sdkClient.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/clients/sdkClient.ts Outdated Show resolved Hide resolved
ar-conmit
ar-conmit reviewed Jul 1, 2022
View reviewed changes
packages/relay/src/lib/clients/sdkClient.ts Outdated
@@ -184,6 +184,11 @@ export class SDKClient {
}

async submitEthereumTransaction(transactionBuffer: Uint8Array): Promise<TransactionResponse> {
if (transactionBuffer.length > 4096) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: the transaction size can be moved to a constant var

@natanasow
Copy link
Collaborator Author

Blocked, waiting for PR to be merged.

@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2022 •
edited
Loading

Codecov Report

Merging #281 (b529a2b) into main (b1a9155) will not change coverage.
The diff coverage is 0.00%.

❗ Current head b529a2b differs from pull request most recent head e8c7fd8. Consider uploading reports for the commit e8c7fd8 to get more accurate results

@@           Coverage Diff           @@
##             main     #281   +/-   ##
=======================================
  Coverage   56.31%   56.31%           
=======================================
  Files           9        9           
  Lines         824      824           
  Branches      130      130           
=======================================
  Hits          464      464           
  Misses        334      334           
  Partials       26       26
Impacted Files Coverage Δ
packages/relay/src/lib/clients/sdkClient.ts 6.30% <0.00%> (ø)

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 b1a9155...e8c7fd8. Read the comment docs.

ar-conmit
ar-conmit previously approved these changes Jul 4, 2022
View reviewed changes
Copy link
Collaborator

@ar-conmit ar-conmit 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 added enhancement New feature or request limechain P1 labels Jul 4, 2022
@Nana-EC Nana-EC added this to the 0.3.0 milestone Jul 4, 2022
Nana-EC
Nana-EC reviewed Jul 4, 2022
View reviewed changes
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.

LG, also waiting on the skd-js pull to go through

packages/server/tests/acceptance/rpc.spec.ts Outdated Show resolved Hide resolved
@natanasow natanasow marked this pull request as ready for review July 6, 2022 11:54
ar-conmit
ar-conmit previously approved these changes Jul 6, 2022
View reviewed changes
Copy link
Collaborator

@ar-conmit ar-conmit 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 previously approved these changes Jul 6, 2022
View reviewed changes
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.

LG

@natanasow @Nana-EC
Bump the sdk version
5bd54cf 
Signed-off-by: nikolay <[email protected]>
@natanasow @Nana-EC
Edit test
b193b49 
Signed-off-by: nikolay <[email protected]>
@Nana-EC Nana-EC dismissed stale reviews from ar-conmit and themself via b193b49 July 6, 2022 17:42
@Nana-EC Nana-EC force-pushed the 15-implement-eth_sendRawTransaction-for-large-ethereum-txns branch from 9c5b2d9 to b193b49 Compare July 6, 2022 17:42
Nana-EC
Nana-EC previously approved these changes Jul 6, 2022
View reviewed changes
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.

LGTM

@Nana-EC
Fix package lock
69ad602 
Signed-off-by: Nana-EC <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Jul 6, 2022

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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Nana-EC Nana-EC self-requested a review July 6, 2022 19:03
@Nana-EC Nana-EC merged commit fb6e217 into main Jul 6, 2022
@Nana-EC Nana-EC deleted the 15-implement-eth_sendRawTransaction-for-large-ethereum-txns branch July 6, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nana-EC Nana-EC Nana-EC approved these changes

@ar-conmit ar-conmit ar-conmit left review comments

@Ivo-Yankov Ivo-Yankov Awaiting requested review from Ivo-Yankov

Assignees

@natanasow natanasow

Labels
enhancement New feature or request limechain P1
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.

Implement eth_sendRawTransaction for large Ethereum TXns
4 participants
@natanasow @codecov-commenter @ar-conmit @Nana-EC