Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Add swap and decode_len to DoubleMap #3749

Closed
rockbmb wants to merge 5 commits into from
Closed

Add swap and decode_len to DoubleMap #3749

rockbmb wants to merge 5 commits into from

Conversation

@rockbmb
Copy link
Contributor

@rockbmb rockbmb commented Oct 2, 2019

Closes #3741.

cc @thiolliere

@parity-cla-bot
Copy link

parity-cla-bot commented Oct 2, 2019

It looks like @rockbmb signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@rockbmb
Copy link
Contributor Author

rockbmb commented Oct 3, 2019

Still missing some tests, but the basic idea is there.

kianenigma
kianenigma reviewed Oct 3, 2019
View reviewed changes
srml/support/src/storage/mod.rs Outdated

/// Read the length of the value in a fast way, without decoding the entire value.
///
/// `T` is required to implement `Codec::DecodeLength`.
Copy link
Contributor

@kianenigma kianenigma Oct 3, 2019

Choose a reason for hiding this comment

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

Suggested change
/// `T` is required to implement `Codec::DecodeLength`.
/// `V` is required to implement `Codec::DecodeLength`.

@kianenigma
Copy link
Contributor

kianenigma commented Oct 3, 2019 •
edited
Loading

Still missing some tests, but the basic idea is there.

Indeed, and thanks for the PR! 👍

@gui1117
Copy link
Contributor

gui1117 commented Oct 4, 2019

yes great, you can add some basic test in srml/support/src/lib.rs or srml/support/src/storage/storage_items.rs.

@rockbmb
Copy link
Contributor Author

rockbmb commented Oct 7, 2019

Still need to figure out what structures need to be present for tests, WIP.

bkchr
bkchr suggested changes Oct 9, 2019
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Looks good in general, but requires some fixing to the tests and some other nitpicks.

srml/support/src/storage/mod.rs Outdated
KArg2: EncodeLike<K2>;

/// Swap the values of two key-pairs.
fn swap<KArg1, KArg2>(key11: KArg1, key12: KArg2, key21: KArg1, key22: KArg2)
Copy link
Member

@bkchr bkchr Oct 9, 2019

Choose a reason for hiding this comment

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

I would like to see 4 generic parameters here. Otherwise we force the user to have 2 times the same type. That is not something we wanted to achieve with EncodeLike.

srml/support/src/lib.rs Outdated
DataDM::insert(0, 3, 2);
DataDM::insert(0, 4, 3);

let collect = || DataDM::enumerate().collect::<Vec<_>>();
Copy link
Member

@bkchr bkchr Oct 9, 2019

Choose a reason for hiding this comment

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

We don't have enumerate() for double maps.

Copy link
Contributor Author

@rockbmb rockbmb Nov 2, 2019

Choose a reason for hiding this comment

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

What does DataDM have available? I see that it's generated by a decl_storage! macro, a little hard to know what it actually is.

Copy link
Contributor

@gui1117 gui1117 Nov 10, 2019

Choose a reason for hiding this comment

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

decl_storage doc is here

so it implements StorageDoubleMap

Copy link
Contributor Author

@rockbmb rockbmb Nov 10, 2019

Choose a reason for hiding this comment

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

Hm, I see - so we can't iterate over all (key1, key2, value) triplets.

srml/support/src/storage/storage_items.rs Outdated
assert_eq!(OptionLinkedMapVec::decode_len(0), Ok(0));

// Double map
assert_eq!(DoubleMapVec::get(0), vec![]);
Copy link
Member

@bkchr bkchr Oct 9, 2019

Choose a reason for hiding this comment

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

get() takes two parameters.

@bkchr
Copy link
Member

bkchr commented Oct 17, 2019

@rockbmb any update?

@rockbmb
Copy link
Contributor Author

rockbmb commented Oct 17, 2019

@bkchr I'll circle back to this, handling other things at the moment.

@gui1117
Copy link
Contributor

gui1117 commented Nov 1, 2019

@rockbmb if you want to I can finish it

@rockbmb
Copy link
Contributor Author

rockbmb commented Nov 2, 2019 •
edited
Loading

@thiolliere thanks, I appreciate it but it's not that much work so I'll take care of it.

If there's something I do not understand, I'll make sure to ping.

@bkchr
Copy link
Member

bkchr commented Nov 10, 2019

Any update?

@rockbmb
Copy link
Contributor Author

rockbmb commented Nov 10, 2019

@bkchr could you reply to #3749 (comment) please?

@gavofyork
Copy link
Member

gavofyork commented Nov 22, 2019

any update here?

@gavofyork gavofyork added A0-please_review Pull request needs code review. and removed A5-grumble labels Nov 22, 2019
@rockbmb
Copy link
Contributor Author

rockbmb commented Nov 23, 2019

If it's urgent to have this merged, I don't mind if it's taken over.

@bkchr
Copy link
Member

bkchr commented Nov 23, 2019

It's more like that we want to merge it :)

@gavofyork
Copy link
Member

gavofyork commented Dec 1, 2019

If it's urgent to have this merged, I don't mind if it's taken over.

@rockbmb if you want to I can finish it

@thiolliere maybe it's worth you finishing it...

@gui1117 gui1117 mentioned this pull request Dec 2, 2019
Merged
@gui1117
Copy link
Contributor

gui1117 commented Dec 2, 2019

I fixed tests there #4264
Thanks for the PR @rockbmb , while fixing the test I see the error message from decl_storag was indeed confusing maybe you were stuck there. Although this error message will be improved once parsing in decl_storage is rewritten.

@gui1117 gui1117 closed this Dec 2, 2019
@rockbmb rockbmb deleted the double-map-functions branch December 2, 2019 11:16
bkchr pushed a commit that referenced this pull request Dec 2, 2019
@gui1117 @bkchr
Add swap and decode_len to DoubleMap finishing #3749 (#4264)
a5739a9 
* Add `swap` and `decode_len` to `DoubleMap`

*  Add tests to `swap` and `decode_len` for `DoubleMap` (WIP)

* Address review comments

* Remove function that is not in scope

* fix test

* better naming
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@gui1117 gui1117 Awaiting requested review from gui1117

2 more reviewers

@kianenigma kianenigma kianenigma left review comments

@bkchr bkchr bkchr requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make feature of DoubleMap on par with Maps

7 participants

@rockbmb @parity-cla-bot @kianenigma @gui1117 @bkchr @gavofyork @paritytech-ci