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!: enforce single pfb per blobtx #1195

Merged
merged 7 commits into from
Jan 10, 2023

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Jan 9, 2023

Overview

part of #388, split apart from #1154

This PR refactors enforcement of only allowing a single PFB per sdk.Tx and, per feedback in #1154, adds a new block validity rule where all transactions must be decodable!

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

@MSevey MSevey requested a review from a team January 9, 2023 00:47
@evan-forbes evan-forbes self-assigned this Jan 9, 2023
@evan-forbes evan-forbes added ABCI modifies an ABCI method consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules x/blob item is directly relevant to the blob module labels Jan 9, 2023
@evan-forbes evan-forbes added this to the Incentivized Testnet milestone Jan 9, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #1195 (7584357) into main (4e3b307) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1195      +/-   ##
==========================================
- Coverage   48.17%   48.11%   -0.06%     
==========================================
  Files          72       72              
  Lines        4056     4061       +5     
==========================================
  Hits         1954     1954              
- Misses       1930     1935       +5     
  Partials      172      172              
Impacted Files Coverage Δ
app/process_proposal.go 0.00% <0.00%> (ø)
x/blob/types/blob_tx.go 19.69% <0.00%> (+2.12%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cmwaters
cmwaters previously approved these changes Jan 9, 2023
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.

Just minor stuff but otherwise looks good

app/process_proposal.go Outdated Show resolved Hide resolved
x/blob/types/errors.go Outdated Show resolved Hide resolved
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
@MSevey MSevey requested a review from a team January 9, 2023 14:36
cmwaters
cmwaters previously approved these changes Jan 9, 2023
rootulp
rootulp previously approved these changes Jan 9, 2023
x/blob/types/blob_tx.go Outdated Show resolved Hide resolved
x/blob/types/errors.go Outdated Show resolved Hide resolved
@evan-forbes evan-forbes dismissed stale reviews from rootulp and cmwaters via 639d446 January 10, 2023 01:14
evan-forbes and others added 3 commits January 9, 2023 19:14
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Still LGTM

@evan-forbes evan-forbes merged commit a9b97e6 into main Jan 10, 2023
@evan-forbes evan-forbes deleted the evan/enforce-single-PFB-per-blobtx branch January 10, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABCI modifies an ABCI method consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules x/blob item is directly relevant to the blob module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants