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

Implement data directory for #8 #24

Merged
merged 43 commits into from Apr 3, 2019
Merged

Implement data directory for #8 #24

merged 43 commits into from Apr 3, 2019

Conversation

jfinkhaeuser
Copy link
Contributor

@jfinkhaeuser jfinkhaeuser commented Mar 30, 2019

Fixes #8 when merged.

Following the whitepaper is a little more complex than the issue bullet list describes.

Missing:

  • tests
  • a README, describing the flows (from the whitepaper)

@jfinkhaeuser jfinkhaeuser mentioned this pull request Mar 30, 2019
@jfinkhaeuser
Copy link
Contributor Author

jfinkhaeuser commented Apr 1, 2019

I'm running into some issues with the tests. Basically, I can't find a way to create account IDs in my mocks. There doesn't seem to be any sort of constructor for T::AccountId that I can find. When I try to initialize it from an u64 (because in the mocks, that's what it is), there's an error that an associated type was expected. When I try to initialize it from an empty struct {}, I get errors that the type is u64, so I can't use that constructor.

It's really frustrating to say the least.

It seems as if rust is smart enough to be type safe with its abstract types, but then only selectively smart enough to know what concrete type it's dealing with.

@jfinkhaeuser
Copy link
Contributor Author

@mnaamani @siman I realize that tests are still missing, but if you have feedback already, that'd be great!

@mnaamani
Copy link
Contributor

mnaamani commented Apr 1, 2019

@jfinkhaeuser

This doesn't work?

const Alice: u64 = 1;

or

let Alice = 1 as u64;

Overkill but this could work also:

let Alice = <Test as system::Trait>::AccountId::sa(1);

@jfinkhaeuser
Copy link
Contributor Author

Looks in the mocks, it complains that there's an issue with the As trait not being implemented or something. I can paste the error. It's a bit of a mystery to me, as the things that work elsewhere don't seem to work in the mock.

@jfinkhaeuser
Copy link
Contributor Author

Casting via as keyword doesn't work; there's an error saying that you can only use it on primitive types. You and I know that AccountId is a u64, but apparently the rust compiler doesn't within this context.

@mnaamani
Copy link
Contributor

mnaamani commented Apr 1, 2019

Yes you will need to have the As trait in scope.

use runtime_primitives::traits::As

@jfinkhaeuser
Copy link
Contributor Author

error[E0599]: no function or associated item named `sa` found for type `<T as srml_system::Trait>::AccountId` in the current scope
  --> src/storage/mock.rs:66:35
   |
66 |             origin: T::AccountId::sa(1),
   |                     --------------^^
   |                     |
   |                     function or associated item not found in `<T as srml_system::Trait>::AccountId`

and

error[E0308]: mismatched types
  --> src/storage/mock.rs:66:21
   |
66 |             origin: 1 as u64,
   |                     ^^^^^^^^ expected associated type, found u64
   |
   = note: expected type `<T as srml_system::Trait>::AccountId`
              found type `u64`

respectively.

The As trait should be in scope; if I use it explicitly, there's another error again.

@jfinkhaeuser
Copy link
Contributor Author

As best as I can read it, the issue is that system::Trait doesn't actually define what AccountId is, so the compiler can't infer that as should exist. Because what we really are using is the Test struct for which the trait is implemented.

I also experimented with Test::AccountId to no avail.

@mnaamani
Copy link
Contributor

mnaamani commented Apr 1, 2019

let Alice = <Test as system::Trait>::AccountId::sa(1);

Test not T

@jfinkhaeuser
Copy link
Contributor Author

Same difference, the first of the two errors above happen.

@jfinkhaeuser
Copy link
Contributor Author

I just had another idea, though...

@jfinkhaeuser
Copy link
Contributor Author

Nope, same issue.

src/storage/data_directory.rs Outdated Show resolved Hide resolved
src/storage/data_directory.rs Outdated Show resolved Hide resolved
src/storage/data_directory.rs Outdated Show resolved Hide resolved
src/storage/data_directory.rs Outdated Show resolved Hide resolved
src/storage/data_directory.rs Outdated Show resolved Hide resolved
src/storage/downloads.rs Outdated Show resolved Hide resolved
src/storage/downloads.rs Outdated Show resolved Hide resolved
src/storage/downloads.rs Outdated Show resolved Hide resolved
src/storage/downloads.rs Outdated Show resolved Hide resolved
src/storage/downloads.rs Show resolved Hide resolved
Co-Authored-By: jfinkhaeuser <jens@finkhaeuser.de>
src/storage/downloads.rs Outdated Show resolved Hide resolved
@jfinkhaeuser jfinkhaeuser marked this pull request as ready for review April 2, 2019 14:45
@jfinkhaeuser
Copy link
Contributor Author

I think this is done, actually. Let me know if there's still something pressing.

@siman siman merged commit 1c86595 into Joystream:development Apr 3, 2019
mnaamani added a commit to mnaamani/joystream that referenced this pull request Mar 20, 2020
Proposals refactoring: Abstention -> Abstain, test runtime code update, fix test-all script
mnaamani pushed a commit to mnaamani/joystream that referenced this pull request Jun 19, 2020
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

3 participants