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

Add a full node RPC endpoint, get_mempool_item_by_coin_name #15625

Closed

Conversation

kimsk
Copy link
Contributor

@kimsk kimsk commented Jun 26, 2023

Purpose:

Implement a new endpoint to get mempool item by a coin name mentioned in #14810.

Testing Notes:

pytest

tests/core/test_full_node_rpc/test_coin_name_not_found_in_mempool
tests/core/test_full_node_rpc/test_coin_name_found_in_mempool

manual

found
image
not found
image

@Yakuhito
Copy link
Contributor

Wow - thank you!

@emlowe emlowe added the Added Required label for PR that categorizes merge commit message as "Added" for changelog label Jun 27, 2023
@emlowe
Copy link
Contributor

emlowe commented Jun 27, 2023

Error: The following new issues have been introduced:
chia/rpc/full_node_rpc_api.py:745: error: Function is missing a type annotation for one or more arguments [no-untyped-def]

request: Dict[str,Any] should fix that

@emlowe
Copy link
Contributor

emlowe commented Jun 28, 2023

chia/rpc/full_node_rpc_api.py (88.9%): Missing lines 747
chia/rpc/full_node_rpc_client.py (100%)
tests/core/test_full_node_rpc.py (97.6%): Missing lines 459,542

Easy enough to add a test to test the Exception case on line 747

I suspect the test code can't trigger the if is_overflow_block condition ( I might be inclined to just remove that if check from the test as it doesn't seem relevant to the specific test.)

@Yakuhito
Copy link
Contributor

Hey there! I know I was the one who originally opened an issue, but it seems this endpoint needs to be changed based on #13799. From my understanding, multiple mempool items can now exist that contain the same CoinSpend. I would make the endpoint return a list of those, and name it get_mempool_items_by_coin_name. This could also allow you to delete the if len(mempool_items) == 0 condition (just return an empty list).

@kimsk
Copy link
Contributor Author

kimsk commented Jun 29, 2023

Easy enough to add a test to test the Exception case on line 747. I suspect the test code can't trigger the if is_overflow_block condition ( I might be inclined to just remove that if check from the test as it doesn't seem relevant to the specific test.)

@emlowe good suggestions. I update the tests and add the exception test (i.e., line 747).

@kimsk
Copy link
Contributor Author

kimsk commented Jun 29, 2023

I would make the endpoint return a list of those, and name it get_mempool_items_by_coin_name. This could also allow you to delete the if len(mempool_items) == 0 condition (just return an empty list).

@Yakuhito That was my original implementation 😄. Since 1.8.2 allows multiple mempool items with the same coin id now, I agree that we should update the RPC to return a list as well as its name to get_mempool_items_by_coin_name.

@emlowe is it fine to just use this PR or you prefer closing this one and re-open the new one?

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 17, 2023
@github-actions
Copy link
Contributor

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

@kimsk
Copy link
Contributor Author

kimsk commented Aug 12, 2023

Use this #16019 instead.

@kimsk kimsk closed this Aug 12, 2023
@kimsk kimsk deleted the karlkim.get_mempool_item_by_coin_name branch August 12, 2023 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog merge_conflict Branch has conflicts that prevent merge to main
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants