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: Remove obsolete gas-limited parameters #1584

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leinss
Copy link

@leinss leinss commented Oct 11, 2024

Hi @ZenGround0, @aarshkshah1992

I started with the PLDG cohort and took #1268 as first issue.

I removed the following parameters:

  • PRE_COMMIT_SECTOR_BATCH_MAX_SIZE
  • PROVE_REPLICA_UPDATES_MAX_SIZE (this seems to have been set to PRE_COMMIT_SECTOR_BATCH_MAX_SIZE anyway and not being used elsewhere)
  • DECLARATIONS_MAX

and made sure that the tests still run.

Where there are tests with a batch size param, I used 256 as hardcoded value, as this was the initial value for it.

Please let me know if this is the correct approach I am going and if there are more obsolete fields.

Thanks!

@Abhay-2811
Copy link
Contributor

@Caruso33 had same CI-related issues try cargo fmt --all and cargo clippy --all then run make check to run CI checks

BTW this is not a comment/review related to the issue or pr, someone from the core team might do that meanwhile you can run the following commands and fix CI-related issues

@leinss
Copy link
Author

leinss commented Nov 5, 2024

seems we have now 2 PRs for the same issue? #1586

@rjan90
Copy link
Contributor

rjan90 commented Jan 8, 2025

Hey! 👋

As part of our cleanup to kick off the year, I'm reviewing all open non-draft pull requests. Could you please do one of the following for your PR?

1. Close it: If it's no longer needed.
2. Mark as Draft: If it needs more work, and add the next steps.
3. Ready for Review: If it's good to go, let me know, and I'll assign a reviewer.

If there's no response in a week, I'll assume it's option 1 and close the PR. If you have any questions, just let me know.

Thanks for your help in keeping things organized, and I appreciate your contributions!

@rvagg
Copy link
Member

rvagg commented Jan 8, 2025

Oh, dupe of #1586 (or #1586 is a dupe of this since it's later), sorry @leinss I guess we didn't do any assigning or keep people in the loop on this. There's some further discussion in #1586 that add context to this and a path to getting one of these merged.

Let's keep this one open until we resolve the need for a FIP and then figure out which one to use as a base.

Thanks for the contribution regardless @leinss! Sorry we neglected this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📌 Triage
Development

Successfully merging this pull request may close these issues.

4 participants