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

Migrate from near_sdk::collections to near_sdk::store #37

Merged
merged 3 commits into from
Nov 5, 2022

Conversation

mooori
Copy link
Contributor

@mooori mooori commented Nov 4, 2022

This only affects the Acl plugin, other plugins aren't using near_sdk::collections.

Motivation

near_sdk::collections will be put under the legacy feature flag and its successor is near_sdk::store. Once a contract uses a plugin based on collections, it will require efforts to migrate that contract to a new plugin version based on store. To avoid that, let's migrate near-plugins itself to near_sdk::store before any plugins are used in production.

near_sdk::store

Main differences to near_sdk::collections are described in the last section of this post.

Feature flag unstable

To use store it's currently still required to enable the unstable feature for near_sdk. Seems like store will be stabilized in sdk version 4.1 (currently it's at 4.0.0).

Implementation details

Usage patterns are more idiomatic for rust

For instance maps now have get_mut which allows removing patterns like get element -> modify it -> write back to map.

Clones

Using store without changing any function signatures requires cloning values at various places since some near-sdk functions previously handling references now handle owned values and vice versa. For instance collections::UnorderedSet::insert has parameter element: &T while store::UnorderedSet::insert has parameter element: T.

A follow-up PR could change some signatures of internal functions of the Acl plugin to avoid some of these clones.

Review

It might be more convenient to look at the commits individually.

@mooori mooori marked this pull request as ready for review November 4, 2022 10:50
@mooori mooori requested a review from mfornet November 4, 2022 10:50
Copy link
Contributor

@mfornet mfornet left a comment

Choose a reason for hiding this comment

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

It is unfortunate the introduction of new clones. Mostly because I expect, inside these methods, the object is fully serialized, so it is first cloned, then serialized. This is feedback we can pass on to the near-sdk-rs team.

Copy link

@sept-en sept-en left a comment

Choose a reason for hiding this comment

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

LGTM! Looks cleaner now. The only thing is to perhaps wait for API to stabilize, as we want to have a minimal impact on future upgrades.

@mooori
Copy link
Contributor Author

mooori commented Nov 4, 2022

I've created #38 for changing internal function signature to get rid of some of the new clones. I should have more insights afterwards and will leave another comment then.


We're using UnorderedSet and UnordereMap from near_sdk::store. It seems like in our version of near-sdk (4.0.0) they're unstable just due to outstanding optimizations, testing and docs. They should be stabilized in the upcoming 4.1.1. without changes affecting near-plugins (#922, #924).

So I assume it's fine to merge this PR now. @sept-en wdyt?

@sept-en
Copy link

sept-en commented Nov 4, 2022

@mooori makes sense to me. This PR is ok to merge for sure.

@mooori mooori merged commit 7454bbc into Near-One:master Nov 5, 2022
@mooori mooori deleted the sdk-store branch November 5, 2022 12:38
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.

4 participants