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

Update cip-6.md title and add summary table #76

Merged
merged 2 commits into from
Feb 15, 2024
Merged

Conversation

ebuchman
Copy link
Collaborator

@ebuchman ebuchman commented Feb 8, 2024

In the spirit of CIP-13, added a single-row table summarizing the new parameter.

Would be good to set a standard to have this table in any CIP that introduces a new parameter or modifies one so it can easily be copied into CIP-13 if/when it's approved.

In this vein it wasn't entirely clear to me what the param store will be called. This CIP mentions it will be a new param.Subspace but doesn't name it, so I'm optimistically calling it gas but feel free to correct. That said it's not clear to me why this shouldn't just go under auth along with eg. auth.TxSizeCostPerByte?

Also updated the title to be a bit more descriptive

Overview

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

In the spirit of CIP-13, added a single-row table summarizing the new parameter.

Would be good to set a standard to have this table in any CIP that introduces a new parameter so it can easily be copied into CIP-13 if/when it's approved.

Also updated the title to be a bit more descriptive
@ebuchman ebuchman changed the title Update cip-6.md Update cip-6.md title and add summary table Feb 8, 2024
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Thanks @ebuchman! Yeah I think a table makes it clearer

cips/cip-6.md Outdated Show resolved Hide resolved
Co-authored-by: Callum Waters <[email protected]>
Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

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

LGTM if @cmwaters approves 🤝

@jcstein jcstein merged commit e10d057 into main Feb 15, 2024
1 check passed
@jcstein jcstein deleted the ebuchman-patch-1 branch February 15, 2024 19:18
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.

3 participants