-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat(ext): add app-side library for NEAR #374
Conversation
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis update introduces enhanced functionality to the NEAR protocol SDK, including new structures for handling RPC query requests and responses. It implements a Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
@miraclx shouldn't this be using the generic fetch call instead being part of the client SDK? I'm a bit confused. |
I think this is because |
But for the blockchain calls we should implement some logic to verify it right? And also allow developers to bake verification into the app if needed on the service level? @miraclx can you provide info here? |
it's part of the client SDK to consolidate the available host functions and abstractions for correct use however, it's marked However, the client libraries will continue to use it, cautiously.
That is correct, at least for transaction data, blocks are trickier since we'll have to fetch the whole block (and all it's transactions) to do this verification, but we can provide the option. |
Yes, we should use it with caution and provide feedback to people who use fetch directly. That is why we should close this PR and implement the near fetch using the generic fetch with some verification. |
we shouldn't need to close it, just expand on it |
…d-near-app-side-library
…d-near-app-side-library
…d-near-app-side-library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (14)
- Cargo.toml (3 hunks)
- apps/only-peers/Cargo.toml (1 hunks)
- crates/primitives/Cargo.toml (1 hunks)
- crates/primitives/src/common.rs (1 hunks)
- crates/primitives/src/events.rs (1 hunks)
- crates/primitives/src/identity.rs (1 hunks)
- crates/runtime/src/logic.rs (1 hunks)
- crates/sdk/libs/near/Cargo.toml (1 hunks)
- crates/sdk/libs/near/src/error.rs (1 hunks)
- crates/sdk/libs/near/src/jsonrpc.rs (1 hunks)
- crates/sdk/libs/near/src/lib.rs (1 hunks)
- crates/sdk/libs/near/src/query.rs (1 hunks)
- crates/sdk/libs/near/src/types.rs (1 hunks)
- crates/sdk/libs/near/src/views.rs (1 hunks)
Files skipped from review as they are similar to previous changes (11)
- Cargo.toml
- apps/only-peers/Cargo.toml
- crates/primitives/Cargo.toml
- crates/primitives/src/common.rs
- crates/primitives/src/events.rs
- crates/primitives/src/identity.rs
- crates/runtime/src/logic.rs
- crates/sdk/libs/near/Cargo.toml
- crates/sdk/libs/near/src/error.rs
- crates/sdk/libs/near/src/jsonrpc.rs
- crates/sdk/libs/near/src/views.rs
Additional comments not posted (2)
crates/sdk/libs/near/src/types.rs (2)
1-11
: Code organization and usage ofserde_with
for serialization.The use of
serde_with
for Base64 encoding is appropriate for binary data types likeStoreValue
andStoreKey
. This ensures efficient and correct serialization and deserialization of binary data.
45-58
: Review and optimize serialization settings forFunctionArgs
.The
FunctionArgs
struct uses Base64 encoding, which is suitable for binary data. However, ensure that all uses of this struct require such encoding to avoid unnecessary performance overhead.Verification successful
Base64 encoding for
FunctionArgs
is appropriate.The usage of
FunctionArgs
in bothcrates/sdk/libs/near/src/views.rs
andcrates/application/src/lib.rs
involves handling binary data, justifying the need for Base64 encoding.
crates/sdk/libs/near/src/views.rs
: Used inCallFunction
variant ofQueryRequest
with a field nameargs_base64
.crates/application/src/lib.rs
: Used in constructing aQueryRequest::CallFunction
request where JSON data is converted to bytes.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all usages of `FunctionArgs` to ensure Base64 encoding is necessary. # Test: Search for the struct usage. Expect: Only contexts where binary data is handled. rg --type rust $'FunctionArgs'Length of output: 486
Script:
#!/bin/bash # Search for the context around the usage of `FunctionArgs` in `crates/sdk/libs/near/src/views.rs`. rg -A 10 -B 10 'FunctionArgs' crates/sdk/libs/near/src/views.rsLength of output: 1012
Script:
#!/bin/bash # Search for the context around the usage of `FunctionArgs` in `crates/application/src/lib.rs`. rg -A 10 -B 10 'FunctionArgs' crates/application/src/lib.rsLength of output: 1500
@coderabbitai pause |
Actions performedReviews paused. |
…d-near-app-side-library
@its-saeed Please add a test plan and a summary. Can you prepare the docs? This must have public docs @MatejVukosav |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, now let's add an example that tests the library for basic things
like your test case in the description, but it's own application, you should receive specific variables from the inputs, and not hard coded
Should it be in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miraclx let's keep current scope. If the code is ok please approve the PR. @its-saeed review the comments you are adding in the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving to unblock, let's get this merged 🎉
Co-authored-by: Xabi Losada <[email protected]>
…etwork/core into 370-add-near-app-side-library
Summary
We need to develop a high-level library that wraps around the low-level
fetch
function to call query methods of the NEAR blockchain in a deterministic way. This library should abstract the complexities of the low-level function and provide a simple and intuitive interface for developers.Test plan
For internal tests, I added this function to the only-peers app to call NEAR query methods:
Checklist
Summary by CodeRabbit
New Features
Client
struct for interacting with NEAR Protocol RPC endpoints.Enhancements
Cargo.toml
tocalimero-sdk-near
.foo
to theOnlyPeers
struct for retrieving account balance.Bug Fixes
peer_id
field type inPeerJoinedPayload
struct tolibp2p_identity::PeerId
.Refactor
libp2p
tolibp2p-identity
with additional features.Chores