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

dev: use from_reader and to_writer_pretty from serde_json, remove reading from memory to string #562

Merged
merged 5 commits into from
Sep 29, 2023
Merged

dev: use from_reader and to_writer_pretty from serde_json, remove reading from memory to string #562

merged 5 commits into from
Sep 29, 2023

Conversation

bajpai244
Copy link
Contributor

@bajpai244 bajpai244 commented Sep 13, 2023 •
edited
Loading

Resolves: #561

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Testing

What is the new behavior?

As mentioned in #561 , we were often reading first from a file to string, and then passing it to serde_json.

serde_json has the following methods:

Which can allow reading and writing directly from and to a Reader and a Writer respectively.

This PR also fixes some variables names, where it seems fit.

Does this introduce a breaking change?

  • Yes
  • No

@bajpai244 bajpai244 force-pushed the dev/remove-reading-writing-to/from-string branch 4 times, most recently from fd6c402 to 0a50f51 Compare September 13, 2023 11:48
@codecov
Copy link

codecov bot commented Sep 13, 2023 •
edited
Loading

Codecov Report

Patch coverage: 83.58% and project coverage change: +49.69% 🎉

Comparison is base (71506a8) 23.17% compared to head (6e022c7) 72.87%.
Report is 238 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff             @@
##             main     #562       +/-   ##
===========================================
+ Coverage   23.17%   72.87%   +49.69%     
===========================================
  Files           9       45       +36     
  Lines        1247     4140     +2893     
===========================================
+ Hits          289     3017     +2728     
- Misses        958     1123      +165
Files Changed Coverage Δ
crates/core/src/mock/serde.rs 0.00% <0.00%> (ø)
crates/core/src/models/transaction.rs 89.51% <ø> (ø)
crates/core/src/models/transaction_receipt.rs 97.02% <ø> (ø)
crates/eth-rpc/src/api/alchemy_api.rs 100.00% <ø> (ø)
crates/eth-rpc/src/api/eth_api.rs 100.00% <ø> (ø)
crates/eth-rpc/src/api/net_api.rs 100.00% <ø> (ø)
crates/eth-rpc/src/api/web3_api.rs 100.00% <ø> (ø)
crates/eth-rpc/src/config.rs 100.00% <ø> (ø)
crates/eth-rpc/src/lib.rs 93.33% <ø> (ø)
crates/eth-rpc/src/main.rs 0.00% <ø> (ø)
... and 35 more

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

@bajpai244 bajpai244 force-pushed the dev/remove-reading-writing-to/from-string branch 2 times, most recently from dd2b5c0 to 423cb6f Compare September 13, 2023 11:52
ftupas
ftupas suggested changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@ftupas ftupas left a comment

Choose a reason for hiding this comment

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

Great work so far, just some comments and nits on unwraps 🙂

crates/hive-utils/src/hive/genesis.rs Outdated Show resolved Hide resolved
crates/hive-utils/src/hive/genesis.rs Outdated Show resolved Hide resolved
crates/hive-utils/src/hive/genesis.rs Outdated Show resolved Hide resolved
crates/test-utils/src/bin/dump-katana.rs Outdated Show resolved Hide resolved
crates/test-utils/src/bin/dump-katana.rs Outdated Show resolved Hide resolved
@bajpai244 bajpai244 force-pushed the dev/remove-reading-writing-to/from-string branch from f0a1c75 to 23ac17d Compare September 13, 2023 15:35
ftupas
ftupas previously requested changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@ftupas ftupas left a comment

Choose a reason for hiding this comment

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

Minor comments 🙂

crates/hive-utils/src/hive/genesis.rs Outdated Show resolved Hide resolved
crates/test-utils/src/bin/dump-katana.rs Outdated Show resolved Hide resolved
crates/test-utils/src/bin/dump-katana.rs Outdated Show resolved Hide resolved
crates/test-utils/src/deploy_helpers.rs Outdated Show resolved Hide resolved
crates/test-utils/src/deploy_helpers.rs Outdated Show resolved Hide resolved
@bajpai244 bajpai244 force-pushed the dev/remove-reading-writing-to/from-string branch 5 times, most recently from afdfac3 to 60668d2 Compare September 14, 2023 06:28
ClementWalter
ClementWalter previously approved these changes Sep 14, 2023
View reviewed changes
@ClementWalter
Copy link
Member

need to rebase and fix clippy

@ftupas ftupas mentioned this pull request Sep 17, 2023
Merged
9 tasks
@bajpai244 bajpai244 force-pushed the dev/remove-reading-writing-to/from-string branch 3 times, most recently from 4994ad0 to 9b69e0a Compare September 19, 2023 04:08
@Eikix
Copy link
Member

Eikix commented Sep 19, 2023

Can be rebased now

@bajpai244 bajpai244 force-pushed the dev/remove-reading-writing-to/from-string branch 2 times, most recently from 85eb416 to 632c038 Compare September 19, 2023 10:16
@bajpai244 bajpai244 force-pushed the dev/remove-reading-writing-to/from-string branch from 08d7eec to aa7247f Compare September 25, 2023 06:27
@Eikix Eikix added this pull request to the merge queue Sep 29, 2023
Merged via the queue into kkrt-labs:main with commit 4010adf Sep 29, 2023
9 checks passed
anukkrit149 pushed a commit to karnotxyz/kakarot-rpc that referenced this pull request Aug 9, 2024
@Eikix
Update README.md (kkrt-labs#562)
ca1a3ff 
<!--- Please provide a general summary of your changes in the title
above -->

<!-- Give an estimate of the time you spent on this PR in terms of work
days. Did you spend 0.5 days on this PR or rather 2 days? -->

Time spent on this PR:

## Pull request type

<!-- Please try to limit your pull request to one type, submit multiple
pull requests if needed. -->

Please check the type of change your PR introduces:

- [ ] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe):

## What is the current behavior?

<!-- Please describe the current behavior that you are modifying, or
link to a relevant issue. -->

Resolves #<Issue number>

## What is the new behavior?

<!-- Please describe the behavior or changes that are being added by
this PR. -->

-
-
-

## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eikix Eikix Eikix approved these changes

@ftupas ftupas ftupas left review comments

@ClementWalter ClementWalter ClementWalter left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Write data to file directly instead of writing it first as an in-memory string
4 participants
@bajpai244 @ClementWalter @Eikix @ftupas