Skip to content
This repository was archived by the owner on Aug 23, 2021. It is now read-only.

Adds endpoint for client.queryStorageDeal #67

Merged
merged 4 commits into from
Aug 30, 2019
Merged

Adds endpoint for client.queryStorageDeal #67

merged 4 commits into from
Aug 30, 2019

Conversation

hazmatzo
Copy link
Contributor

@hazmatzo hazmatzo commented Aug 6, 2019 •
edited by alanshaw
Loading

Adds endpoint to query a storage deal

Solves issue #16

Endpoint
/api/client/query-storage-deal

Description
Query a storage deal's status

Arguments

  • arg [string]: CID of deal to query Required: yes.

Response
On success, the call to this endpoint will return with 200 and the following body:

{
  "State":7,
  "Message":"",
  "ProposalCid":
    {
      "/":"zDPWYqFD8CNXu7Mo9qPSUANbTK2vi9vJBnvavF9S1pVGPHafVHpT"
    },
  "ProofInfo":
    {
      "SectorID":1,
      "CommitmentMessage":
        {
          "/":"zDPWYqFCtHkWNkE2p6t6TeV1sPP5kbnKc5ajUhMVV8xvrw1u5F1R"
        },
      "PieceInclusionProof":"EiAbbOy4pChsCYqFYA6qJaUJYStlnwYMdQPHZX7YBkVXDD6vgmGTPnWrcdA9M0oAXQCzOq735YKySLUoTI6pAw=="
    },
  "Signature":"c2lnbmF0dXJycmVlZQ=="
}

resolves #16

@hazmatzo hazmatzo force-pushed the api/client/query-storage-deal branch from 1554803 to 9a042cc Compare August 6, 2019 23:19
@hazmatzo
Copy link
Contributor Author

hazmatzo commented Aug 6, 2019

@alanshaw

@mishmosh mishmosh requested a review from alanshaw August 7, 2019 18:19
@hazmatzo hazmatzo force-pushed the api/client/query-storage-deal branch from 9a042cc to 627ee20 Compare August 7, 2019 21:12
alanshaw
alanshaw suggested changes Aug 9, 2019
View reviewed changes
API.md
"PieceInclusionProof":"EiAbbOy4pChsCYqFYA6qJaUJYStlnwYMdQPHZX7YBkVXDD6vgmGTPnWrcdA9M0oAXQCzOq735YKySLUoTI6pAw=="
},
"Signature":"c2lnbmF0dXJycmVlZQ=="
}
Copy link
Member

Choose a reason for hiding this comment

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

To make this more idiomatic for JS users we should convert the root and ProofInfo to camel case using the toCamel utility.

e.g. res.ProofInfo.PieceInclusionProof should be accessed like res.proofInfo.pieceInclusionProof

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

API.md Outdated
"Message":"",
"ProposalCid":
{
"/":"zDPWYqFD8CNXu7Mo9qPSUANbTK2vi9vJBnvavF9S1pVGPHafVHpT"
Copy link
Member

Choose a reason for hiding this comment

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

This is an artifact of CID encoding in IPLD, to make it easier to work with we replace CID objects like this with the CID string. e.g.

https://github.com/filecoin-project/js-filecoin-api-client/blob/2c0e82fd8cfe7b959b1eb067a5d8e9960d2a16fe/src/cmd/actor/ls.js#L14

@alanshaw
Copy link
Member

alanshaw commented Aug 9, 2019

It would be nice to get an e2e test for this (and other commands you're submitting). The current ones run against an already started daemon.

It would be even more rad if you were to tackle #69

@hazmatzo
Adds endpoint for client.queryStorageDeal
de388a3 
* '/api/client/query-storage-deal'

Query a storage deal's status

Arguments
* arg [string]: CID of deal to query Required: yes.

Response
On success, the call to this endpoint will return with 200 and the following body:

{
  "State":7,
  "Message":"",
  "ProposalCid":
    {
      "/":"zDPWYqFD8CNXu7Mo9qPSUANbTK2vi9vJBnvavF9S1pVGPHafVHpT"
    },
  "ProofInfo":
    {
      "SectorID":1,
      "CommitmentMessage":
        {
          "/":"zDPWYqFCtHkWNkE2p6t6TeV1sPP5kbnKc5ajUhMVV8xvrw1u5F1R"
        },
      "PieceInclusionProof":"EiAbbOy4pChsCYqFYA6qJaUJYStlnwYMdQPHZX7YBkVXDD6vgmGTPnWrcdA9M0oAXQCzOq735YKySLUoTI6pAw=="
    },
  "Signature":"c2lnbmF0dXJycmVlZQ=="
}
@hazmatzo hazmatzo force-pushed the api/client/query-storage-deal branch from 627ee20 to de388a3 Compare August 12, 2019 16:54
@hazmatzo
Copy link
Contributor Author

It would be nice to get an e2e test for this (and other commands you're submitting). The current ones run against an already started daemon.

It would be even more rad if you were to tackle #69

Hey @alanshaw, I'd be happy to tackle it but would probably need some additional context. I'm happy to start comments on the issue, but any chance you can chat?

@hazmatzo hazmatzo force-pushed the api/client/query-storage-deal branch from cbeff10 to 68b7174 Compare August 12, 2019 18:01
@hazmatzo
Changes the API to be camelcase
8b896fb 
* In order to be more idiomatic for JavaScript
* Modified the incoming proposalCid, which was historically encoded in IPLD
@hazmatzo hazmatzo force-pushed the api/client/query-storage-deal branch from 68b7174 to 8b896fb Compare August 12, 2019 18:05
alanshaw
alanshaw suggested changes Aug 12, 2019
View reviewed changes
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Nearly there!

src/cmd/client/query-storage-deal.js

if (storageDeal.proofInfo) {
storageDeal.proofInfo = toCamel(storageDeal.proofInfo)
}
Copy link
Member

Choose a reason for hiding this comment

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

Need to fix the CID here too!

Suggested change
}
}
if (storageDeal.proofInfo && storageDeal.proofInfo.commitmentMessage) {
storageDeal.proofInfo.commitmentMessage = storageDeal.proofInfo.commitmentMessage['/']
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it as part of the commit.

@alanshaw
Copy link
Member

Hey @alanshaw, I'd be happy to tackle it but would probably need some additional context. I'm happy to start comments on the issue, but any chance you can chat?

I'm in #filecoin on freenode if you'd like to discuss!

alanshaw
alanshaw suggested changes Aug 12, 2019
View reviewed changes
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Any chance you can fix up the linting warnings also? 🙏 https://circleci.com/gh/filecoin-project/js-filecoin-api-client/94

@hazmatzo
Copy link
Contributor Author

Fixed up the linting and added the commitmentMessage in the new format. That had been an outstanding question in my mind.

I'm on #filecoin as hazmatzo. Give me 10 minutes and I can probably chat.

@hazmatzo
Copy link
Contributor Author

Oh, I'm on #filecoin Slack. I haven't joined the IRC...

@hazmatzo
Copy link
Contributor Author

@alanshaw Hey! Is there anything else?

Relatedly, I looked into doing the e2e tests for any of these new endpoints, but to me it seems like I'd need the ability to spin up a new daemon for any of them?

@mishmosh
Copy link
Collaborator

@alanshaw are you comfortable with merging this now that linting and other original comments have been fixed?

I believe @hazmatzo and @alanshaw chatted in more depth about e2e testing -- @hazmatzo could you document that in a separate issue?

@alanshaw
Copy link
Member

I believe @hazmatzo and @alanshaw chatted in more depth about e2e testing -- @hazmatzo could you document that in a separate issue?

@mishmosh I made some progress towards this today and published https://www.npmjs.com/package/go-filecoin-dep. The AI is for me to document in #69 the requirements for a module that is a utility to spawn a fresh filecoin node that we can use for e2e testing.

@hazmatzo this is great - thanks a lot ❤️

@alanshaw alanshaw merged commit 45655d0 into filecoin-shipyard:master Aug 30, 2019
@mishmosh mishmosh mentioned this pull request Sep 6, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alanshaw alanshaw alanshaw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add command client.queryStorageDeal
3 participants
@hazmatzo @alanshaw @mishmosh