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

docs(cip-13): add NetworkMinGasPrice to params list #115

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ninabarbakadze
Copy link
Member

@ninabarbakadze ninabarbakadze marked this pull request as draft March 27, 2024 12:47
@rootulp
Copy link
Collaborator

rootulp commented Mar 27, 2024

Looks like markdownlint is failing here. Is the plan to hold off on merging this until the Lemongrass hardfork executes on mainnet?

@ninabarbakadze ninabarbakadze marked this pull request as ready for review March 27, 2024 15:17
@celestia-bot celestia-bot requested a review from ebuchman March 27, 2024 15:17
@ninabarbakadze
Copy link
Member Author

Looks like markdownlint is failing here. Is the plan to hold off on merging this until the Lemongrass hardfork executes on mainnet?

waiting till lemongrass is executed seems reasonable to me. should i convert this into a draft till then?

@rootulp
Copy link
Collaborator

rootulp commented Mar 27, 2024

should i convert this into a draft till then?

Sounds good to me. That way we don't accidentally merge it early.

@jcstein jcstein marked this pull request as draft March 28, 2024 20:29
@cmwaters
Copy link
Contributor

cmwaters commented Apr 23, 2024

Looks like markdownlint is failing here. Is the plan to hold off on merging this until the Lemongrass hardfork executes on mainnet?

My impression was that all these CIPs should be finalized before the lemongrass hardfork is finalized hence this PR should be merge ASAP

Finalizing does not mean implemented nor in production just that the design has been agreed upon

@rootulp
Copy link
Collaborator

rootulp commented Apr 23, 2024

This PR doesn't change anything about the design. It adds a param to the living document of params in CIP-13. IMO we don't need to merge this PR ASAP because Lemongrass is still months away.

If you want to merge this PR ASAP, then I propose adding versions to the relevant params in CIP-13 and then we can merge a modified version of this PR that states GlobalMinGasPrice is only relevant to app version 2.

@ninabarbakadze ninabarbakadze marked this pull request as ready for review July 24, 2024 10:25
@ninabarbakadze ninabarbakadze changed the title docs(cip-13): add GlobalMinGasPrice to params list docs(cip-13): add NetworkMinGasPrice to params list Jul 24, 2024
@ninabarbakadze
Copy link
Member Author

Overview

I updated the definition and the value to latest. @rootulp @cmwaters

Should we merge? @YazzyYaz @jcstein

@rootulp
Copy link
Collaborator

rootulp commented Jul 24, 2024

IMO we shouldn't merge b/c then the params in this table don't reflect what is on mainnet. I think the intention of this table is that it's a living document of what is on mainnet. If we want to preserve that, we could have two tables like what is done here:

  1. https://celestiaorg.github.io/celestia-app/specs/parameters_v1.html
  2. https://celestiaorg.github.io/celestia-app/specs/parameters_v2.html

Or we wait until v2.0.0 gets deployed on mainnet

@jcstein
Copy link
Member

jcstein commented Jul 24, 2024

I'll mark as draft again as to prevent an accidental early merge :)

@jcstein jcstein marked this pull request as draft July 24, 2024 14:40
@rootulp
Copy link
Collaborator

rootulp commented Jul 24, 2024

[tangent] I think it's going to be a slight maintenance burden to keep this document up to date with what is on mainnet. We already have: https://celestiaorg.github.io/celestia-app/specs/parameters.html so I don't see CIP-13 adding much value.

@jcstein
Copy link
Member

jcstein commented Jul 24, 2024

Do the parameters update based on what is on mainnet already? https://celestiaorg.github.io/celestia-app/specs/parameters.html

@rootulp
Copy link
Collaborator

rootulp commented Jul 24, 2024

Nope that's manually updated. I'm trying to minimize the number of places we need to update b/c I think it could lead to inconsistencies.

Also the source of truth is the params actually on mainnet so we could generate a site that queries the params from a mainnet node and displays them.

@jcstein
Copy link
Member

jcstein commented Jul 24, 2024

I'm trying to minimize the number of places we need to update b/c I think it could lead to inconsistencies.

seems like a safe idea. I wonder if maybe we can turn this CIP into one that lives on the site somehow...similarly to how, for example, https://celestia.cool pulls latest mempool info from a consensus endpoint

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

Successfully merging this pull request may close these issues.

Update CIP-13 to add minfee.GlobalMinGasPrice
5 participants