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

Overhaul Key API #9

Closed
Robbepop opened this issue Jan 1, 2019 · 8 comments
Closed

Overhaul Key API #9

Robbepop opened this issue Jan 1, 2019 · 8 comments
Labels
A-ink_storage [ink_storage] Work Item B-design Designing a new component, interface or functionality.

Comments

@Robbepop
Copy link
Collaborator

Robbepop commented Jan 1, 2019

Key API

The Key API as of now mainly consists of

  • Key::with_offset(key: Key, offset: u32) -> Key
  • Key::with_chunk_offset(key: Key, offset: u32) -> Key
  • Key::store(&mut self, bytes: &[u8])
  • Key::load(&self) -> Vec<u8>
  • Key::clear(&mut self)

Especially the Key::with_offset and Key::with_chunk_offset APIs are not intuitive.
A better design would be to use the std::ops::Add and std::ops::AddAssign traits and implement them for u32 (replaces uses of Key::with_offset), u64 (replaces uses of Key::with_chunk_offset) and also some versions for i32 and i64 to go in both directions.
Due to technical implementation issues the unsigned variants could be implemented a bit more efficient.

For completeness one could also think about u128 and i128 support since Key is 256-bit anyway.

Also Key::store, Key::load and Key::clear could be marked unsafe since those operations are normally not safe using the raw Key abstraction and are only made safe by the many alternatives, like SyncCell, Value, Vec, etc.

@Robbepop Robbepop added B-design Designing a new component, interface or functionality. A-ink_storage [ink_storage] Work Item labels Jan 1, 2019
@Robbepop
Copy link
Collaborator Author

Robbepop commented Jan 1, 2019

This commit adds std::ops::{Add, AddAssign} implementations for Key for u32 and u64.

  • Signed versions will follow.
  • Concerns about adding another std::ops::Sub and std::ops::SubAssign implementation.
  • Also we could add something similar to a ptrdiff with std::ops::Sub<Key> for Key impl. However, what return type should it have?

@Robbepop
Copy link
Collaborator Author

Robbepop commented Jan 1, 2019

Commits 8880390 and 10e5f15 remove Key::with_chunk_offset and Key::with_offset in favor of the new core::ops::{Add, AddAssign} implementations.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Jan 2, 2019

Commit 42c490b marks Env::load as unsafe fn since it compares itself with a pointer deref in Rust which is also known as being an unsafe operation.

Changes to Env::store and Env::clear will follow with the same reasoning.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Jan 2, 2019

Commit bb2e8ee marks the other Env::{store, clear} fns as unsafe as the previous commit.

This shall guide users of pdsl_core to use the safer cell, chunk and collections abstractions.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Jan 2, 2019

Commit 89b0be5 adds core::ops::Sub impl for Key and the KeyDiff type.
Look at commit 4509914 on how to use this new interface.

Currently missing are some unit tests for KeyDiff.

Edit: Commit b91fb4a adds simple unit tests for KeyDiff.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Jan 3, 2019

Missing pieces:

  • impl core::ops::Sub<{u32, u64, u128}> for Key
  • impl core::ops::Add<u128> for Key
  • impl core::ops::Add<{i32, i64, i128}> for Key. Note that this hasn't been made yet since the unsigned variants can be implemented a bit more efficiently. This might not be transparent to users so could lead to a slight performance degradation when using signed instead of unsigned after implementing this. Besides that these trait implementations are for convenience. This requires a byte_utils::sign_extend procedure to sign extend the i32, i64 or i128 to the 32 byte of a key in order to subtract or add from or to it.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Jan 3, 2019

Besides other things this commit adds core::ops::{Sub, SubAssign} implementations for Key.

Also during implementation we found out that core::ops::{Add,AddAssign,Sub,SubAssign} implementations for RHS = {i32,i64,i128} do not make sense and are semantically difficult to articulate on Key instances so it is better to leave them away and stay with the unsigned variants that have a semantically clear usage story.

So after removing this work item what is missing is implementations of core::ops::{Add,AddAssign,Sub,SubAssign} ofu128 for Key and also a conversion impl from KeyDiff to u128.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Jan 7, 2019

The remaining parts have been implemented in 966d29d, bc711cb and 09b7bbd.

@Robbepop Robbepop closed this as completed Jan 7, 2019
TriplEight added a commit that referenced this issue Mar 5, 2021
9: Bors branch r=TriplEight a=TriplEight

this is to tailor bors config

Co-authored-by: Denis P <denis.pisarev@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ink_storage [ink_storage] Work Item B-design Designing a new component, interface or functionality.
Projects
None yet
Development

No branches or pull requests

1 participant