Replace Proxy with Object API by avichalp · Pull Request #86 · recallnet/ipc

avichalp · 2024-06-06T10:14:43Z

This PR deals with the following:

Removes the obsolete HTTP endpoints, which should not be since we have SDK now. The exception here is the os-get and os-list endpoints. These are still in use. The SDK uses these endpoints to download the file. In this PR, I have added a new GET endpoint for SDK to download files. First, the SDK needs to move over to the new endpoint, and then these two could be deleted in an upcoming PR. Due to these two endpoints, the proxy wallet and sequence have not been removed yet. But it will be removed in a subsequent PR.
Renames all "proxy" references in provisioning scripts etc except for the proxy-wallet which will be handled in the next PR.

Fixes #78

avichalp · 2024-06-06T10:29:41Z

fendermint/app/src/cmd/object_api.rs

-}
+    // If it is a HEAD request, we don't need to send the body
+    // but we still need to send the Content-Length header
+    if method == "HEAD" {


Added a new HEAD /objects method to get the size of file. It will help with adding a progress bar in the sdk: recallnet/rust-recall#38

avichalp · 2024-06-06T11:23:39Z

ipc/provider/src/lotus/json.rs

 use serde_json::{json, Value};

 /// A trait to convert a type to a JSON value.
+#[allow(dead_code)]


@sanderpick: This code is coming from upstream but it is not used anywhere anymore. Not sure if we should ignore the warning or remove it?

sanderpick · 2024-06-06T16:10:29Z

fendermint/app/config/default.toml

+[object_api]

-[proxy.listen]
+[object_api.listen]


how about just objects?

and everywhere is where "object api" is used... i think it could just be "objects". easier to read

sanderpick · 2024-06-06T16:10:59Z

fendermint/app/config/default.toml

+[object_api.listen]
 # Only accept local connections by default.
-host = "127.0.0.1"
+host = "0.0.0.0"


I think it's a good idea to keep this local in the default config. Env vars can open it up.

sanderpick · 2024-06-06T19:20:44Z

fendermint/app/src/cmd/object_api.rs

-    gas_limit: Option<u64>,
-    broadcast_mode: Option<BroadcastMode>,
+async fn handle_object_download(
+    cid: Cid,


does this method just download by cid? is there a way for the api to reject downloading an object if it is not resolved?

i think, we would need to call GetObject here to know if it is resolved or not. the SDK already knows if the object is resolved and it only makes the download request if it is resolved. but that means that users can potentially download unresolved objects which we should not allow?

right, because otherwise a user could submit to the object API and then just not send the txn to comet, and still be able to download the file from this node.

but you can do that by embedding the SDK into this api right?

yeah we can use the SDK to make the GetObject call again to check the resolved status.

I am wondering now weather SDK and CLI should have a separate method for downloading detached objects. because with one get method currently, we call GetObject on the actor and if it is a detached object we call the objects API which will again call the GetObject to get the resolve status.

not sure if it would be bad dev UX to have two methods and leaking the details about attached and detached objects.

i think one method in the SDK is pretty nice. do we need to call get from the SDK if the object API is doing it for the resolved check?

I think, this is will sorted now that we are make all objects detached.

sanderpick · 2024-06-09T20:41:33Z

I forgot to mention, in a draft PR (now superseded by #90), I added a change to skip the "only hash" step in the object api when adding a file. It cuts in half the time it takes to upload an object: db4f160#diff-678770e4084ddb1a610b9550c305bfa4364ab9c43251de3e194dbef200c09cd9

Mentioned in an issue here #84. Can you add that to this PR?

avichalp · 2024-06-11T11:40:46Z

@sanderpick: should be merge this with objects GET call still in there? This PR migrates the SDK to use objects GET call for now. Once we have the required functionality in S3-API service, I will remove this endpoint and the remaining proxy endpoints (get and list) from here.

sanderpick · 2024-06-11T15:04:23Z

Sounds good to me.

Looks like I caused some conflicts here with the newer PRs that got merged.

Signed-off-by: avichalp <[email protected]>

sanderpick · 2024-06-11T15:22:49Z

infra/fendermint/scripts/objects.toml

  --env FM_PROXY_SECRET_KEY=/data/${NODE_NAME}/${PROXY_PRIV_KEY_PATH} \
  --env FM_PROXY_ACCOUNT_KIND=ethereum \
  --env FM_PROXY_BROADCAST_MODE=sync \
  --env FM_PROXY_SEQUENCE=0 \


Should these also be removed?

I left these because they were being used while creating a TransClient that we still use that in the objects GET endpoint. We can remove after SDK starts using the s3 service.

https://github.com/amazingdatamachine/ipc/blob/0e62386400550cdc901af2ba44ae26869cb45d5a/fendermint/app/src/cmd/rpc.rs#L329-L341

sanderpick · 2024-06-11T15:26:13Z

fendermint/app/options/src/objects.rs


 #[derive(Subcommand, Debug, Clone)]
-pub enum ProxyCommands {
+pub enum ObjectsCommands {


Do we need still need the args: super::rpc::TransArgs in here?

Hrm, looks like we do as long as we're using the TransClient from cmd::rpc. Once we move to the SDK we can use VoidSigner, ie, no private key, sequence, etc. required.

sanderpick · 2024-06-11T15:46:36Z

fendermint/app/src/cmd/objects.rs

-                    .and(warp::header::optional::<u64>("X-ADM-GasLimit"))
-                    .and(warp::header::optional::<BroadcastMode>("X-ADM-BroadcastMode"))
-                    .and_then(handle_os_upload);
+                let objects_download = warp::path!("v1" / "objects" / Address / String)


I think you need to handle the full path here like in the original download method: warp::path!("v1" / "objects" / Address / ..), and then handle tail: Tail in the handler.

It looks like that the tail is being used for list in the original API? But SDK already does list/query call directly. In this new API I added a String param to get the key. Using the objectstore's Address and the key it finds out the object to download.

It's used for listing when tail is empty or it end in slash, e.g., foo/, foo/bar/. But it's used to get the object otherwise, e.g., foo, foo/bar. You need to match on "v1" / "objects" / Address / .. to access recursive paths like foo/bar. IIRC otherwise you'll get a 404 not found HTTP error.

Oh! that makes sense. Changing and testing with nested paths now.

Signed-off-by: avichalp <[email protected]>

sanderpick

lgtm!

sanderpick · 2024-06-11T23:30:35Z

Merging so we can get started on new deployment.

avichalp commented Jun 6, 2024

View reviewed changes

avichalp requested a review from sanderpick June 6, 2024 10:36

avichalp marked this pull request as ready for review June 6, 2024 10:36

avichalp commented Jun 6, 2024

View reviewed changes

avichalp force-pushed the avichalp/object-head branch from bf4c07b to 0c8304e Compare June 6, 2024 18:28

sanderpick reviewed Jun 6, 2024

View reviewed changes

avichalp force-pushed the avichalp/object-head branch 3 times, most recently from 1064add to f2de6d9 Compare June 11, 2024 09:52

avichalp mentioned this pull request Jun 11, 2024

Add progressbar in download recallnet/rust-recall#44

Merged

avichalp added 10 commits June 11, 2024 23:44

Remove obselete proxy code

4bd4b2b

Signed-off-by: avichalp <[email protected]>

Rename proxy to object_api

aab6739

Signed-off-by: avichalp <[email protected]>

rename proxy cargo make files

ea4dac0

Signed-off-by: avichalp <[email protected]>

silence more clippy warnings

2e11e92

Signed-off-by: avichalp <[email protected]>

fix fmt

c6581fe

Signed-off-by: avichalp <[email protected]>

Rename objects-api -> objects

340940b

Signed-off-by: avichalp <[email protected]>

Remove unrequired clippy ignore macro

ecb915b

Signed-off-by: avichalp <[email protected]>

Use address and key to download object

8a6dfd5

Signed-off-by: avichalp <[email protected]>

Fix clippy

ee997c8

Signed-off-by: avichalp <[email protected]>

Remove double hashing

e6a57e7

Signed-off-by: avichalp <[email protected]>

avichalp force-pushed the avichalp/object-head branch from f2de6d9 to e6a57e7 Compare June 11, 2024 15:45

sanderpick reviewed Jun 11, 2024

View reviewed changes

avichalp added 2 commits June 12, 2024 01:23

Fix nested key bug

526697f

Signed-off-by: avichalp <[email protected]>

Fix lint

ab89dc5

Signed-off-by: avichalp <[email protected]>

sanderpick approved these changes Jun 11, 2024

View reviewed changes

sanderpick merged commit d6dcc55 into develop Jun 11, 2024

sanderpick deleted the avichalp/object-head branch June 11, 2024 23:30

avichalp mentioned this pull request Jun 12, 2024

Fix from address in os_get call #98

Merged

Conversation

avichalp commented Jun 6, 2024 • edited by sanderpick Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanderpick commented Jun 9, 2024

Uh oh!

avichalp commented Jun 11, 2024

Uh oh!

sanderpick commented Jun 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanderpick left a comment

Choose a reason for hiding this comment

Uh oh!

sanderpick commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

avichalp commented Jun 6, 2024 •

edited by sanderpick

Loading