search

Near-One / near-plugins

Implementation of common patterns used for NEAR smart contracts.
Creative Commons Zero v1.0 Universal
27 stars 12 forks source link

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

Closed mooori closed 1 year ago

mooori commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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