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

πŸ’Ώ Storage: Pin / Unpin object #126

Merged
merged 15 commits into from
Mar 9, 2023
Merged

πŸ’Ώ Storage: Pin / Unpin object #126

merged 15 commits into from
Mar 9, 2023

Conversation

bdeneux
Copy link
Contributor

@bdeneux bdeneux commented Feb 28, 2023

πŸ“ Description

Implement the PinObject and UnpinObject execute message.

❓ Question

What do we do if when we execute PinObject on object that has been already pinned ? The same question for UnpinObject message ?

Now, if we try to pin an already pinned object, the contract return successful response simulating that the object has just been pinned. Otherwise if we try to unpin an object that is already not pinned to the sender, the contract return also a successful response, by doing this on the unpin object and to limit access to the state, there is no verification on the objects state to check if object exist when trying to unpin object. So if we try to unpin object that does not exist, the contract return successfully because the (ObjectId, senderAddr) tuple doesn't exist in the pins state.

πŸ”— Link

Wait #123 before merge

@bot-anik
Copy link
Member

bot-anik commented Feb 28, 2023

size-limit report πŸ“¦

Path Size
target/wasm32-unknown-unknown/release/cw_template.wasm 178.1 KB (0%)
target/wasm32-unknown-unknown/release/cw_logic_sample.wasm 203.87 KB (0%)
target/wasm32-unknown-unknown/release/cw_storage.wasm 261.73 KB (-0.3% πŸ”½)

@bdeneux bdeneux requested review from ccamel and amimart and removed request for ccamel March 1, 2023 09:58
@bdeneux bdeneux self-assigned this Mar 1, 2023
@bdeneux bdeneux requested a review from ccamel March 1, 2023 09:58
@amimart
Copy link
Member

amimart commented Mar 2, 2023

@bdeneux Regarding the behavior when pinning already pinned object and unpinning not yet pinned object I agree with you, as it changes nothing in the state, to consider those messages successful, making them as no-op.

@ccamel Your opinion about that? We'll need to update the specifications accordingly.

@bdeneux
Copy link
Contributor Author

bdeneux commented Mar 2, 2023

@bdeneux Regarding the behavior when pinning already pinned object and unpinning not yet pinned object I agree with you, as it changes nothing in the state, to consider those messages successful, making them as no-op.

@ccamel Your opinion about that? We'll need to update the specifications accordingly.

@amimart @ccamel Sorry, my bad, after checking, the specification already indicate this behavior :

/// # PinObject
    /// PinObject pins the object in the bucket for the considered sender. If the object is already pinned
    /// for the sender, this is a no-op.
    /// While an object is pinned, it cannot be removed from the storage.
/// # UnpinObject
    /// UnpinObject unpins the object in the bucket for the considered sender. If the object is not pinned
    /// for the sender, this is a no-op.
    /// The object can be removed from the storage if it is not pinned anymore.

So it's ok for the PinObject. My question now is about the UnpinObject, what do you think if we try to unpin a not found object id, since the pin is not found on the Pins state, this will be also a no-op, do we need instead to return that the object is not found (keeping in mind that this need to ask the state twice instead of once if the object exist) ?

@amimart
Copy link
Member

amimart commented Mar 2, 2023

@bdeneux In the case we try to unpin a non existing object, I would tend to make it an error case, even if it has a little more cost than considering it a no-op. Because a successful unpin of an object could suggest the existence of it.

@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2023

Codecov Report

Merging #126 (57327db) into main (55b02d7) will increase coverage by 1.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
+ Coverage   91.45%   92.76%   +1.30%     
==========================================
  Files          11       11              
  Lines         199      235      +36     
==========================================
+ Hits          182      218      +36     
  Misses         17       17              
Impacted Files Coverage Ξ”
contracts/cw-storage/src/state.rs 100.00% <ΓΈ> (ΓΈ)
contracts/cw-storage/src/contract.rs 96.19% <100.00%> (+1.98%) ⬆️
contracts/cw-storage/src/msg.rs 100.00% <100.00%> (ΓΈ)

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bdeneux bdeneux force-pushed the feat/storage-pin-object branch 3 times, most recently from 63ff027 to 7b2d53f Compare March 8, 2023 10:01
@bdeneux bdeneux marked this pull request as ready for review March 8, 2023 16:14
Copy link
Member

@amimart amimart left a comment

Choose a reason for hiding this comment

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

Nice piece! πŸ’ͺ

I just noted small enhancement ways :)

contracts/cw-storage/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw-storage/src/contract.rs Outdated Show resolved Hide resolved
@bdeneux bdeneux requested a review from amimart March 9, 2023 08:14
Copy link
Member

@amimart amimart left a comment

Choose a reason for hiding this comment

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

Sounds pretty great! πŸš€

Copy link
Member

@ccamel ccamel left a comment

Choose a reason for hiding this comment

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

LGTM! πŸ‘

@bdeneux bdeneux merged commit df7196a into main Mar 9, 2023
@bdeneux bdeneux deleted the feat/storage-pin-object branch March 9, 2023 13:55
@bot-anik
Copy link
Member

πŸŽ‰ This PR is included in version 1.0.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

@bot-anik
Copy link
Member

πŸŽ‰ This PR is included in version 1.0.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants