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

Move default values for blob params to appconsts module #993

Closed
rahulghangas opened this issue Nov 10, 2022 · 4 comments · Fixed by #1031
Closed

Move default values for blob params to appconsts module #993

rahulghangas opened this issue Nov 10, 2022 · 4 comments · Fixed by #1031
Assignees
Labels
chore optional label for items that follow the `chore` conventional commit

Comments

@rahulghangas
Copy link
Contributor

rahulghangas commented Nov 10, 2022

Summary

Default values for blob params are currently hardcoded and should be moved to appconsts module

@rahulghangas rahulghangas added enhancement New feature or request chore optional label for items that follow the `chore` conventional commit and removed enhancement New feature or request labels Nov 10, 2022
@rahulghangas rahulghangas self-assigned this Nov 17, 2022
@rootulp
Copy link
Collaborator

rootulp commented Nov 17, 2022

Naive question but why do we want to define these default parameters in the appconsts module if the associated parameter is configured in the blob module?

I assume the end-goal is that the defaults are only used to set the initial value of the parameter in the blob module and after that the parameters are subject to modification through governance. The value that should be used throughout the application is therefore the parameter in the blob module not the default in appconsts.

@rahulghangas
Copy link
Contributor Author

This was done because I wanted to do the changes incrementally (since it's going to break app in a few different places). PR resolving #1032 will replace all usages of default values to paramstore reads

@rootulp
Copy link
Collaborator

rootulp commented Nov 18, 2022

I don't follow why this issue is desirable as an incremental step. If the MaxSquareSize param is defined in the blob module, then I would expect the initial value (i.e the default) for this parameter to also be defined in the blob module.

Since the initial value (i.e the default) is an implementation detail of the blob module, it can be un-exported. Only the current MaxSquareSize param needs to be exported from the blob module.

@evan-forbes
Copy link
Member

I'm fine with moving these, but I also definitely see the reasoning behind keeping them in the blob module since that's where we're planning on keeping these params

rahulghangas added a commit that referenced this issue Dec 9, 2022
Moves default values for min/max swaure size and gas per blob byte to
appconsts module.
Renames `MinSquareSize` and `MaxSquareSize` in appconsts module to
`DefaultMinSquareSize` and `DefaultMaxSquareSize`

- [x] closes #993

Co-authored-by: Rootul P <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore optional label for items that follow the `chore` conventional commit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants