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.

Sign up for GitHub

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

Jump to bottom

[CHIA-691] simplify MempoolItem #18143

Merged
merged 1 commit into from
Jul 8, 2024
Merged

[CHIA-691] simplify MempoolItem #18143

merged 1 commit into from
Jul 8, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Jun 11, 2024 •
edited
Loading

Purpose:

Simplify MempoolItem, InternalMempoolItem and their uses.

Currently MempoolItem and InternalMempoolItem contain an NPCResult field. the NPCResult field is essentially a sum-type of SpendBundleConditions or an error. However, MempoolItems never contain the error. This patch replaces the NPCResult with the success-state, SpendBundleConditions.

Current Behavior:

MempoolItem and InternalMempoolItem contain error states that are never engaged.

New Behavior:

MempoolItem and InternalMempoolItem are simpler, since the SpendBundleconditions exist unconditionally, it's not optional.

Testing Notes:

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jun 11, 2024
@arvidn arvidn changed the title simplify MempoolItem [CHIA-691] simplify MempoolItem Jun 11, 2024
@arvidn arvidn marked this pull request as ready for review June 12, 2024 11:48
@arvidn arvidn requested a review from a team as a code owner June 12, 2024 11:48
@arvidn arvidn requested a review from AmineKhaldi June 12, 2024 11:48
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jun 21, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jun 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@coveralls-official Coveralls Official
Copy link

coveralls-official bot commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9643415868

Details

  • 45 of 47 (95.74%) changed or added relevant lines in 11 files are covered.
  • 39 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.003%) to 90.881%
Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/full_node/mempool_manager.py 8 10 80.0%
Files with Coverage Reduction New Missed Lines %
chia/data_layer/data_store.py 1 94.37%
chia/wallet/util/wallet_sync_utils.py 1 86.54%
chia/server/node_discovery.py 1 79.96%
chia/server/server.py 1 81.46%
chia/data_layer/data_layer_wallet.py 2 91.93%
chia/full_node/full_node.py 5 86.0%
chia/wallet/wallet_node.py 6 88.36%
chia/_tests/core/util/test_lockfile.py 22 77.73%
Totals Coverage Status
Change from base Build 9619484401: 0.003%
Covered Lines: 100209
Relevant Lines: 110202
💛 - Coveralls

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jun 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed merge_conflict Branch has conflicts that prevent merge to main labels Jun 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

AmineKhaldi
AmineKhaldi previously approved these changes Jun 26, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 3, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 3, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@arvidn @AmineKhaldi
make MempoolItem and InternalMempoolItem contain the SpendBundleCondi…
64a3a6c 
…tions directly, rather than the NPCResult. A mempool item is guaranteed to be valid, so the error state of NPCResult will never be engaged anyway.
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 3, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 3, 2024

File Coverage Missing Lines
chia/full_node/mempool_manager.py 80.0% lines 350, 780
Total Missing Coverage
47 lines 2 lines 95%

@arvidn
Copy link
Contributor Author

arvidn commented Jul 8, 2024

the missing coverage was pre-existing

@arvidn arvidn requested a review from emlowe July 8, 2024 09:40
@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label Jul 8, 2024
@Starttoaster Starttoaster merged commit 6dceabe into main Jul 8, 2024
371 of 372 checks passed
@Starttoaster Starttoaster deleted the mempool-item-conds branch July 8, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AmineKhaldi AmineKhaldi AmineKhaldi approved these changes

@emlowe emlowe emlowe approved these changes

Assignees
No one assigned
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arvidn @AmineKhaldi @emlowe @Starttoaster