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.

Sign up for GitHub

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

Add lock sync primitives #52

Merged
merged 5 commits into from
Jun 15, 2023
Merged

Add lock sync primitives #52

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8302ae9
Add new mesh-sync package for locks and other primitives
ethanfrey Jun 9, 2023
9db7624
Basic implementation of a lockable item for cw-storage-plus
ethanfrey Jun 9, 2023
7bb2158
Basic tests for locking process
ethanfrey Jun 9, 2023
b82d2be
Add tests for using maps
ethanfrey Jun 9, 2023
3f748da
Minor cleanup from PR review
ethanfrey Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Minor cleanup from PR review
  • Loading branch information
@ethanfrey
ethanfrey committed Jun 15, 2023
commit 3f748da1be9fa1f299fe719e0c77de5e245c7046
56 changes: 26 additions & 30 deletions packages/sync/src/locks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,36 +320,32 @@ mod tests_plus {
#[test]
fn map_methods_with_locks() {
let mut store = MockStorage::new();
const PEOPLE: Map<&str, Lockable<Uint128>> = Map::new("people");
const AGES: Map<&str, Lockable<Uint128>> = Map::new("people");

// add a few people
PEOPLE
.save(&mut store, "John", &Lockable::new(Uint128::new(32)))
AGES.save(&mut store, "John", &Lockable::new(Uint128::new(32)))
.unwrap();
PEOPLE
.save(&mut store, "Maria", &Lockable::new(Uint128::new(47)))
AGES.save(&mut store, "Maria", &Lockable::new(Uint128::new(47)))
.unwrap();

// We can edit unlocked person
PEOPLE
.update(&mut store, "John", |p| {
let mut p = p.unwrap_or_default();
*p.write()? += Uint128::new(1);
Ok::<_, PersonError>(p)
})
.unwrap();
AGES.update(&mut store, "John", |p| {
let mut p = p.unwrap_or_default();
*p.write()? += Uint128::new(1);
Ok::<_, PersonError>(p)
})
.unwrap();

// Update works on new values, setting to unlocked by default
PEOPLE
.update(&mut store, "Wilber", |p| {
let mut p = p.unwrap_or_default();
*p.write()? += Uint128::new(2);
Ok::<_, PersonError>(p)
})
.unwrap();
AGES.update(&mut store, "Wilber", |p| {
let mut p = p.unwrap_or_default();
*p.write()? += Uint128::new(2);
Ok::<_, PersonError>(p)
})
.unwrap();

// We can range over them well
let total_age = PEOPLE
let total_age = AGES
.range(&store, None, None, cosmwasm_std::Order::Ascending)
.fold(Ok(Uint128::zero()), |sum, item| {
Ok::<_, PersonError>(sum? + *item?.1.read()?)
Expand All @@ -358,18 +354,18 @@ mod tests_plus {
assert_eq!(total_age, Uint128::new(33 + 47 + 2));

// We can get count
let num_people = PEOPLE
let num_people = AGES
.range(&store, None, None, cosmwasm_std::Order::Ascending)
.count();
assert_eq!(num_people, 3);

// Read-lock John
let mut j = PEOPLE.load(&store, "John").unwrap();
let mut j = AGES.load(&store, "John").unwrap();
j.lock_read().unwrap();
PEOPLE.save(&mut store, "John", &j).unwrap();
AGES.save(&mut store, "John", &j).unwrap();

// We can no longer edit it
let err = PEOPLE
let err = AGES
.update(&mut store, "John", |p| {
let mut p = p.unwrap_or_default();
*p.write()? += Uint128::new(1);
Expand All @@ -379,7 +375,7 @@ mod tests_plus {
assert_eq!(err, PersonError::Lock(LockError::ReadLocked));

// We can still range over all
let total_age = PEOPLE
let total_age = AGES
.range(&store, None, None, cosmwasm_std::Order::Ascending)
.fold(Ok(Uint128::zero()), |sum, item| {
Ok::<_, PersonError>(sum? + *item?.1.read()?)
Expand All @@ -388,18 +384,18 @@ mod tests_plus {
assert_eq!(total_age, Uint128::new(33 + 47 + 2));

// We can get count
let num_people = PEOPLE
let num_people = AGES
.range(&store, None, None, cosmwasm_std::Order::Ascending)
.count();
assert_eq!(num_people, 3);

// Write-lock Wilber
let mut w = PEOPLE.load(&store, "Wilber").unwrap();
let mut w = AGES.load(&store, "Wilber").unwrap();
w.lock_write().unwrap();
PEOPLE.save(&mut store, "Wilber", &w).unwrap();
AGES.save(&mut store, "Wilber", &w).unwrap();

// We cannot range over all
let err = PEOPLE
let err = AGES
.range(&store, None, None, cosmwasm_std::Order::Ascending)
.fold(Ok(Uint128::zero()), |sum, item| {
Ok::<_, PersonError>(sum? + *item?.1.read()?)
Expand All @@ -408,7 +404,7 @@ mod tests_plus {
assert_eq!(err, PersonError::Lock(LockError::WriteLocked));

// We can get count (kind of edge case bug, but I don't think we can change this or it matters)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically, this is correct. The map itself is unlocked, and so, its state can change.

The same with removal of a locked item, I guess? Would be good to add a test for it, just to showcase it.

Copy link
Collaborator

@maurolacy maurolacy Jun 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unless we consider removing a locked item an error, and use a wrapper around remove to try and enforce that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unless we consider removing a locked item an error, and use a wrapper around remove to try and enforce that.

Yeah, it seemed a new LockedMap type would be a whole lot of work, so I was trying to figure out how to do this with minimal intervention.

I think it is correct that count works, but remove should definitely be prohibited. Any ideas how to do so without copying over the entire Map API?

Copy link
Collaborator

@maurolacy maurolacy Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Rust composition over inheritance makes writing this kind of minimally modified containers a pain.

The only way I see around a LockedMap / LockableMap and similar is a Lockable::remove method. It feels unnatural, and enforcement will be responsibility of the user (as there's no way to avoid calling the base / original remove), but it's perhaps preferable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add such a method as helper.
Maybe another PR to add some sort of LockableMap alternative which enforces everything

let num_people = PEOPLE
let num_people = AGES
.range(&store, None, None, cosmwasm_std::Order::Ascending)
.count();
assert_eq!(num_people, 3);
Expand Down