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

Handle large JSON payloads #2512

Closed
wants to merge 12 commits into from
Closed

Conversation

Dzejkop
Copy link

@Dzejkop Dzejkop commented Mar 21, 2022

  • Because this PR includes a bug fix, relevant tests have been included.
  • Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
  • I didn't do anything of this.

@changeset-bot
Copy link

changeset-bot bot commented Mar 21, 2022

⚠️ No Changeset found

Latest commit: e8a42d7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

This PR was marked as stale because it didn't have any activity in the last 30 days. Please excuse us if we didn't have enough time to review it and get it merged. If you are still interested in getting these changes applied, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 27, 2022
@alcuadrado
Copy link
Member

Hey @Dzejkop, this PR is interesting, but can you give more info on the motivation behind it? When did you encounter a problem with the existing code?

Thanks!

@github-actions github-actions bot removed the Stale label Apr 27, 2022
@Dzejkop
Copy link
Author

Dzejkop commented Apr 28, 2022

@alcuadrado sure, it's the same case as #2165.

We're working on a private depoloyment with large limits for contract size. We encountered an error with the JSON.stringify method which would crash if the contract was too large.

@github-actions
Copy link
Contributor

This PR was marked as stale because it didn't have any activity in the last 30 days. Please excuse us if we didn't have enough time to review it and get it merged. If you are still interested in getting these changes applied, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 28, 2022
@fvictorio
Copy link
Member

Not stale

@github-actions github-actions bot removed the Stale label May 30, 2022
@github-actions
Copy link
Contributor

This PR was marked as stale because it didn't have any activity in the last 30 days. Please excuse us if we didn't have enough time to review it and get it merged. If you are still interested in getting these changes applied, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Jul 6, 2022
@citizen-stig
Copy link

@fvictorio Could you please re-open it?

@fvictorio fvictorio reopened this Jul 11, 2022
@fvictorio fvictorio added not-stale and removed Stale labels Jul 11, 2022
@vercel
Copy link

vercel bot commented Jul 11, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hardhat ❌ Failed (Inspect) Jul 11, 2022 at 9:42AM (UTC)
hardhat-storybook ❌ Failed (Inspect) Jul 11, 2022 at 9:42AM (UTC)

@fvictorio
Copy link
Member

Hey @Dzejkop, I'm (finally) looking into this PR, and something I would like to do is to actually test it in a scenario that hits the JSON.stringify/JSON.parse limits.

I tried to create some synthetic examples with a lot of data, but I made solc crash while compiling those. Do you have an example of a scenario where this happens?

@citizen-stig
Copy link

Hello @fvictorio , there's a repo with crafted example: https://github.com/citizen-stig/hardhat-large-contracts-compilation

I've just checked it with commands from READM.md and it reproduces the issue.

Please let me know if you have been able to reproduce it.

@citizen-stig
Copy link

Also details are available in the corresponding issue: #2165

@fvictorio
Copy link
Member

Thanks a lot @citizen-stig, I thought I had seen a reproduction repo in some issue but couldn't find it 🤦‍♂️

@fvictorio
Copy link
Member

Oh, I see. I couldn't reproduce this because it's actually fixed in the latest versions, which save the build info files in a more piece-wise way. We did that for performance reasons, but as a side-effect, it fixed #2165. That's also why there are a lot of merge conflicts in this PR.

I'm going to close this PR now. Sorry for throwing away your work @Dzejkop 😞 But the alternative here is to do this from scratch, and that also means adding a new dependency, which we normally try not to do.

@gitpoap-bot @Dzejkop deserves a gitpoap

@fvictorio fvictorio closed this Dec 14, 2022
@gitpoap-bot
Copy link

gitpoap-bot bot commented Dec 14, 2022

Congrats, @Dzejkop ! You've earned a GitPOAP for your contribution!

GitPOAP: 2022 Hardhat Contributor:

GitPOAP: 2022 Hardhat Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@Dzejkop
Copy link
Author

Dzejkop commented Dec 14, 2022

Thanks for the fix @fvictorio!

And thanks for keeping track of this @citizen-stig

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants