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

Blob #43

Merged
merged 3 commits into from
May 12, 2020
Merged

Blob #43

merged 3 commits into from
May 12, 2020

Conversation

bddap
Copy link
Contributor

@bddap bddap commented May 11, 2020

@bddap bddap requested a review from lovesh May 11, 2020 23:09
@bddap
Copy link
Contributor Author

bddap commented May 11, 2020

@lovesh I don't understand why the develop branch is so out of date. It's 19 commits behind master for some reason.

@bddap
Copy link
Contributor Author

bddap commented May 12, 2020

Couple naming related changes from the spec.

  • dock::blob::Id -> dock::blob::BlobId because polkadotjs can't handle different types with the same name.
  • BlobModule -> BlobStore
  • Querying a blob returns (Did, Vec<u8>) instead of Blob. BlobId is already known because it is the key to the query.

@lovesh
Copy link
Member

lovesh commented May 12, 2020

@lovesh I don't understand why the develop branch is so out of date. It's 19 commits behind master for some reason.

I had started building off an older commit. Will fix this.

UPDATE: Have rebased master

@lovesh lovesh force-pushed the develop branch 2 times, most recently from 8e8ff20 to fb09b4c Compare May 12, 2020 13:07

#[test]
fn add_blob() {
if !in_ext() {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment on the extra call to the test function as one of the earlier PRs. Let's address this with one of the approaches discussed there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to alternate style in 99a5659:

ext().execute_with(|| {
    ...
});

Added new issue #44

use super::*;
use crate::test_common::*;

fn rando_bytes(len: usize) -> Vec<u8> {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a typo in rando_bytes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 99a5659.

rando_bytes -> random_bytes

pub use std::iter::once;

pub type RevoMod = crate::revoke::Module<Test>;
pub type BlobMod = crate::blob::Module<Test>;
Copy link
Member

Choose a reason for hiding this comment

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

BlobMod is not used anywhere else so it should not be added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 99a5659.

Moved BlobMod declaration into blob test module.

@lovesh
Copy link
Member

lovesh commented May 12, 2020

@bddap Minor suggestions, otherwise LGTM.

@lovesh
Copy link
Member

lovesh commented May 12, 2020

Couple naming related changes from the spec.

  • dock::blob::Id -> dock::blob::BlobId because polkadotjs can't handle different types with the same name.
  • BlobModule -> BlobStore
  • Querying a blob returns (Did, Vec<u8>) instead of Blob. BlobId is already known because it is the key to the query.

Can you update the spec as well?

@bddap
Copy link
Contributor Author

bddap commented May 12, 2020

Can you update the spec as well?

Yep docknetwork/planning#4

@bddap bddap merged commit b1fb6b4 into develop May 12, 2020
@bddap bddap deleted the blob branch May 12, 2020 18:55
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