Peerplays_Blockchain/peerplays_migrated

Fork 0

Database API call nft_get_all_tokens() should have limits #458

New issue

Closed

opened 2022-09-30 16:19:43 +00:00 by christophersanborn · 15 comments

christophersanborn commented

2022-09-30 16:19:43 +00:00

(Migrated from gitlab.com)

The Database API function nft_get_all_tokens() will currently return an array of NFT objects encompassing the entire set of existing 1.31.x NFT objects. In the event that the chain has more than a few hundred, few thousand, few million NFTs... this large of a return object may be disruptive and may become a way to DOS a node.

nft_get_all_tokens() — declaration, definition

Suggestion: implement limits

It might be a good idea to add limit and lower bound parameters to the nft_get_all_tokens() call, similar to what exists for list_offers() (declaration, definition). Then the client could request a desired quantity of NFT objects, starting from some initial object id.

The Database API function `nft_get_all_tokens()` will currently return an array of NFT objects encompassing the entire set of existing 1.31.x NFT objects. In the event that the chain has more than a few hundred, few thousand, few million NFTs... this large of a return object may be disruptive and may become a way to DOS a node. `nft_get_all_tokens()` — [declaration](https://gitlab.com/PBSA/peerplays/-/blob/master/libraries/app/include/graphene/app/database_api.hpp#L1013), [definition](https://gitlab.com/PBSA/peerplays/-/blob/master/libraries/app/database_api.cpp#L3020) ### Suggestion: implement limits It might be a good idea to add limit and lower bound parameters to the `nft_get_all_tokens()` call, similar to what exists for `list_offers()` ([declaration](https://gitlab.com/PBSA/peerplays/-/blob/master/libraries/app/include/graphene/app/database_api.hpp#L1025), [definition](https://gitlab.com/PBSA/peerplays/-/blob/master/libraries/app/database_api.cpp#L3108)). Then the client could request a desired quantity of NFT objects, starting from some initial object id.

christophersanborn commented

2022-09-30 16:22:10 +00:00

(Migrated from gitlab.com)

changed title from Database API call {- -}nft_get_all_tokens() should have limits to Database API call nft_get_all_tokens() should have limits

changed title from **Database API call `{- -}nft_get_all_tokens()` should have limits** to **Database API call `nft_get_all_tokens()` should have limits**

christophersanborn commented

2022-09-30 16:22:26 +00:00

(Migrated from gitlab.com)

changed title from Database API call {--}nft_get_all_tokens(){--} should have limits to Database API call nft_get_all_tokens() should have limits

changed title from **Database API call {-`-}nft_get_all_tokens(){-`-} should have limits** to **Database API call nft_get_all_tokens() should have limits**

christophersanborn commented

2022-09-30 16:23:57 +00:00

(Migrated from gitlab.com)

mentioned in issue PBSA/dapps/trade-hands#40

christophersanborn commented

2022-09-30 16:48:10 +00:00

(Migrated from gitlab.com)

mentioned in issue #459

serkixenos commented

2022-10-11 00:27:57 +00:00

(Migrated from gitlab.com)

assigned to @timur.5

timur.5 commented

2022-12-15 12:40:09 +00:00

(Migrated from gitlab.com)

!191

timur.5 commented

2022-12-20 19:12:59 +00:00

(Migrated from gitlab.com)

Testcase:

Use branch
https://gitlab.com/PBSA/peerplays/-/tree/feature/nft_get_metadata_by_owner
In cli_wallet:

gethelp nft_get_all_tokens
gethelp nft_get_tokens_by_owner

and see that they now take "lower_id" and "limit" parameters.

Run 'nft_tests' group of unit tests.
I myself did it by

./chain_test --log_level=message --run_test=nft_tests

Testcase: 0) Use branch https://gitlab.com/PBSA/peerplays/-/tree/feature/nft_get_metadata_by_owner 1) In cli_wallet: ``` gethelp nft_get_all_tokens gethelp nft_get_tokens_by_owner ``` and see that they now take "lower_id" and "limit" parameters. 2) Run 'nft_tests' group of unit tests. I myself did it by ``` ./chain_test --log_level=message --run_test=nft_tests ```

timur.5 commented

2022-12-20 19:13:17 +00:00

(Migrated from gitlab.com)

assigned to @wsalloum and @prandnum

wsalloum commented

2022-12-21 11:13:31 +00:00

(Migrated from gitlab.com)

@timur.5 in which branch is it? it is not working on QA master (develop)

timur.5 commented

2022-12-21 13:20:53 +00:00

(Migrated from gitlab.com)

https://gitlab.com/PBSA/peerplays/-/tree/feature/nft_get_metadata_by_owner

wsalloum commented

2022-12-21 15:56:40 +00:00

(Migrated from gitlab.com)

unlocked >>> gethelp nft_get_all_tokens
gethelp nft_get_all_tokens

Returns all tokens.

Command:
    nft_get_all_tokens limit lower_id

Parameters:
    limit (integer): the maximum number of NFT objects to return (max: 100)
    lower_id (nft_id_type): ID of the first NFT object to include in the
	list.

Returns:
    Returns vector of NFT objects, empty vector if none

unlocked >>> gethelp nft_get_tokens_by_owner
gethelp nft_get_tokens_by_owner

Returns all tokens owned by owner.

Command:
    nft_get_tokens_by_owner owner limit lower_id

Parameters:
    owner (account_id_type): NFT owner account ID
    limit (integer): the maximum number of NFT objects to return (max: 100)
    lower_id (nft_id_type): ID of the first NFT object to include in the
	list.

Returns:
    Returns vector of NFT objects, empty vector if none

``` unlocked >>> gethelp nft_get_all_tokens gethelp nft_get_all_tokens Returns all tokens. Command: nft_get_all_tokens limit lower_id Parameters: limit (integer): the maximum number of NFT objects to return (max: 100) lower_id (nft_id_type): ID of the first NFT object to include in the list. Returns: Returns vector of NFT objects, empty vector if none unlocked >>> gethelp nft_get_tokens_by_owner gethelp nft_get_tokens_by_owner Returns all tokens owned by owner. Command: nft_get_tokens_by_owner owner limit lower_id Parameters: owner (account_id_type): NFT owner account ID limit (integer): the maximum number of NFT objects to return (max: 100) lower_id (nft_id_type): ID of the first NFT object to include in the list. Returns: Returns vector of NFT objects, empty vector if none ```

wsalloum commented

2022-12-21 15:57:19 +00:00

(Migrated from gitlab.com)

@timur.5 can not find chain_test ?

@timur.5 can not find `chain_test` ?

serkixenos commented

2022-12-21 17:46:59 +00:00

(Migrated from gitlab.com)

@wsalloum Code changes were not merged when you tried to tested it. Get the latest code from develop branch and try again.

wsalloum commented

2022-12-21 17:52:14 +00:00

(Migrated from gitlab.com)

I did the test again on https://gitlab.com/PBSA/peerplays/-/tree/feature/nft_get_metadata_by_owner , the first part of the test was ok, but I couldnt find chain_test

bash: ./chain_test: No such file or directory

I did the test again on https://gitlab.com/PBSA/peerplays/-/tree/feature/nft_get_metadata_by_owner , the first part of the test was ok, but I couldnt find `chain_test` ``` bash: ./chain_test: No such file or directory ```

serkixenos commented

2022-12-21 19:48:59 +00:00

(Migrated from gitlab.com)

If you used QA env for testing, tests are not build there. Its enough to verify CI/CD output

https://gitlab.com/PBSA/peerplays/-/jobs/3509787067

Tests are not reporting any errors.

If you used QA env for testing, tests are not build there. Its enough to verify CI/CD output https://gitlab.com/PBSA/peerplays/-/jobs/3509787067 Tests are not reporting any errors.

wsalloum (Migrated from gitlab.com) closed this issue

2022-12-22 08:36:42 +00:00