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 MongodDB project #19

Merged
merged 8 commits into from
Jul 9, 2023
Merged

Conversation

Rishi556
Copy link
Contributor

@Rishi556 Rishi556 commented Mar 30, 2023

projection allows for specific fields to be included or omitted in the response. Adding projection will allow for users to get just the data that they will be using, reducing bandwidth necessary. To use it, project can be added to the contracts.find and contracts.findOne calls within the query as an optional parameter(as well as internal calls). The default behavior is to return all fields.

rpcConfig.allowArbitraryProject is added to the config, with a default value of false. If set to true, it allows project values to be whatever(can have calculations done on result before being sent back to client), but to not overwhelm public nodes, the default configuration just allows 1/0 values(ability to include/exclude fields).

Example call:

curl --location 'https://engine.rishipanthee.com' \
--header 'Content-Type: application/json' \
--data '{
  "jsonrpc": "2.0",
  "method": "contracts.find",
  "params": {"contract" : "nft", "table" : "STARinstances", "query" : {}, "project" : {"account": 1}},
  "id": 2
}'

@Rishi556 Rishi556 marked this pull request as ready for review March 30, 2023 05:50
Copy link
Contributor

@bt-cryptomancer bt-cryptomancer left a comment

Choose a reason for hiding this comment

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

See minor comments about data validation below.

Also, if you are adding support for projections to the RPC, you might as well add it to the version of these functions exposed to the smart contracts API as well, see libs/SmartContracts.js. And then you'll also need a new unit test case (or add on to an existing test case for an arbitrary contract like, say, the tokens contract) to verify the modified smart contracts API works as expected if you include a project field.

libs/Database.js Outdated Show resolved Hide resolved
plugins/JsonRPCServer.js Show resolved Hide resolved
@Rishi556
Copy link
Contributor Author

How would you say is the best way to expose/test it to the SC api? Add projection to a contract and use it there is what I'm thinking, but if you have a better idea, I'll look in that direction.

@bt-cryptomancer
Copy link
Contributor

How would you say is the best way to expose/test it to the SC api? Add projection to a contract and use it there is what I'm thinking, but if you have a better idea, I'll look in that direction.

This sounds like a reasonable approach. What you could do is create a dummy test contract with one action that specifically utilizes this new feature and logs something on success. Then have a unit test case that triggers the action and checks for the expected result in the contract logs.

@bt-cryptomancer
Copy link
Contributor

@Rishi556 any updates on exposing this functionality to the smart contracts API as well as RPC? I don't consider doing that very important, though, as we don't have any immediate planned smart contract uses for it. So if you'd like to consider this task done for now, and hold off on smart contracts API support for another time, I'm fine with that. Please confirm either way.

@Rishi556
Copy link
Contributor Author

Rishi556 commented Jul 9, 2023

I tried writing a dummy contract with a test at one point, but it ended up being a bit messy and I haven't looked at it again since then. I think we can expose it to the smart contracts if its needed at another point, especially since nothing is using it as of now.

@bt-cryptomancer
Copy link
Contributor

I tried writing a dummy contract with a test at one point, but it ended up being a bit messy and I haven't looked at it again since then. I think we can expose it to the smart contracts if its needed at another point, especially since nothing is using it as of now.

Okay sounds good. I'm happy to merge this as-is then, and consider it done for now.

@bt-cryptomancer bt-cryptomancer merged commit 963d431 into hive-engine:qa Jul 9, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants