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

CHIA-1525 Make PreValidationResult take SpendBundleConditions instead of NPCResult #18647

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AmineKhaldi
Copy link
Contributor

Purpose:

PreValidationResult currently uses NPCResult without error possibility in this context, so it's simpler to use SpendBundleConditions directly here.

Current Behavior:

PreValidationResult holds NPCResult and its users have logic to get to SpendBundleConditions from there.

New Behavior:

PreValidationResult holds SpendBundleConditions and its users simply leverage that without needing extra logic.

@AmineKhaldi AmineKhaldi added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup labels Sep 27, 2024
@AmineKhaldi AmineKhaldi self-assigned this Sep 27, 2024
@AmineKhaldi AmineKhaldi force-pushed the prevalidationresults_take_spendbundleconditions branch 2 times, most recently from 44b7508 to 0f7d0e7 Compare September 27, 2024 14:13
Copy link

coveralls-official bot commented Sep 27, 2024

Pull Request Test Coverage Report for Build 11106596781

Details

  • 20 of 20 (100.0%) changed or added relevant lines in 5 files are covered.
  • 23 unchanged lines in 13 files lost coverage.
  • Overall coverage increased (+0.03%) to 90.983%

Files with Coverage Reduction New Missed Lines %
chia/data_layer/data_store.py 1 95.64%
chia/daemon/keychain_proxy.py 1 73.03%
chia/full_node/full_node_api.py 1 82.58%
chia/rpc/rpc_server.py 1 88.82%
chia/farmer/farmer.py 1 72.27%
chia/full_node/full_node_store.py 1 92.09%
chia/timelord/timelord_launcher.py 1 70.55%
chia/daemon/client.py 1 73.33%
chia/_tests/core/util/test_file_keyring_synchronization.py 1 97.14%
chia/consensus/blockchain.py 2 95.26%
Totals Coverage Status
Change from base Build 11074710326: 0.03%
Covered Lines: 102052
Relevant Lines: 112141

💛 - Coveralls

@AmineKhaldi AmineKhaldi marked this pull request as ready for review September 30, 2024 11:41
@AmineKhaldi AmineKhaldi requested a review from a team as a code owner September 30, 2024 11:41
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks good!

@@ -125,12 +124,12 @@ async def validate_block_body(
get_coin_records: Callable[[Collection[bytes32]], Awaitable[List[CoinRecord]]],
block: Union[FullBlock, UnfinishedBlock],
height: uint32,
npc_result: Optional[NPCResult],
conds: Optional[SpendBundleConditions],
Copy link
Contributor

Choose a reason for hiding this comment

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

the documentation of this function, below, should be updated as well. and it should say that this must be set for transaction blocks and must be None for non-transaction blocks

@AmineKhaldi AmineKhaldi force-pushed the prevalidationresults_take_spendbundleconditions branch from 0f7d0e7 to 223d5ff Compare September 30, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants