List of odd APIs

[matklad]

This issue is for me to brain dump all API papercuts I find, just so that I don't forget about them

4.0 Breaking release:

• [x] env::log uses <span class="error">[u8]</span> rather then str Switch log signature to str from bytes #366
• [x] Gas is not a newtype (along with a bunch of other types) refactor(types)!: create newtype for gas #471
• [x] Make Base58PublicKey an enum or struct which does not erase curve type and expose underlying bytes #447 feat: Make CurveType accessible within Base58PublicKey #453
• [x] Remove Base58PublicKey and replace PublicKey with that impl #462 refactor(types): change account id to newtype and deprecate ValidAccountId #448
• [x] Remove ValidAccountId and make AccountId a newtype #446 refactor(types): change account id to newtype and deprecate ValidAccountId #448
• [x] impl TryFrom<String> for CurveType should be FromStr feat(json_types): implement FromStr for Base58CryptoHash and cleanup #398
• [x] VMConfig and VMContext are available when target = wasm32 refactor: replace syscall interactions in wasm32 environment #417
• [x] allocator could be setup by the sdk itself (opt-out via a feature) feat: configure global allocator by default #429
• [x] ValidAccountId shouldn't have TryFrom<&str> or inherent to_string feat(json_types): implement FromStr for ValidAccountId #391
• [x] Update syscall interface and remove BLOCKCHAIN_INTERFACE for wasm32 env refactor: replace syscall interactions in wasm32 environment #417

4.0 Stretch goal:

• "Exporting common crates" feels a bit odd, I think that maybe some of those crates want to be opt-in? In particular, serde_json looks like something that not every contract needs
• [x] add_access_key of the Promise API takes in method names as a Vec<u8> which assumes you know that it's a comma-separated utf8 bytes. Would be nice if it could just take something like impl IntoIterator<Item = &str>, or &<span class="error">[&amp;str]</span> if cautious about using generics here
• [x] refactor!: make blockchain interface concrete type and remove interface trait #451

Future changes:

• near_sdk::CryptoHash and near_sdk::MerkleHash which are similar, but not the same
• init methods require implementing panicking defaults yourself
• &mut self methods can overwrite manually changed contract storage, foot gun when writing migrations.
• Collection's range request has an unergonomic signature: https://doc.rust-lang.org/stable/std/collections/struct.BTreeMap.html#method.range, https://docs.rs/near-sdk/3.1.0/near_sdk/collections/struct.TreeMap.html#method.range (This will be deprecated when transitioning from collections anyway)
• [x] Migrate all collections types to caching and idiomatic variants in store module (doesn't require semver breaking change)
• VMContext fields are all exposed and have ambiguous naming (related Some VMContext attributes can cause confusion #14)
• type Duration = u64 while we have core::time::Duration
• [x] Storage keys have type Vec<u8> when they do not and should not be resized, so they can be switched to Box<<span class="error">[u8]</span>> to save on memory and have some compiler checks to make sure this isn't misused
• Error types (specifically within json_types are Box<dyn std::error::Error> when they could be concrete types
• the whole json_types module is suspicious. If we newtype most of the things, the need for this module should go away. Also if integers are serialized as strings at serialization level, the wrapper types won't be needed (Handle u64 and u128 at the serializer layer rather than via wrapper types? #408)
• [x] Hash functions return a Vec<u8> when they can just return arrays of the length of the hash type (feat: Implement fixed length hash functions and optimize existing imlementations #646)
• gas function exported from MockedBlockchain should probably not be exposed or be renamed to use_gas if kept

[austinabell]

The most prevalent one I've been thinking about has been around the collections API, specifically the map types.

It would be ideal if the API matches BTreeMap/HashMap closely. Specifically, the annoying point was for accessor methods, where the functions accepted &K where you really only need any generic type Q (in std for example) where K: Borrow<Q> which might seem like a small difference, but if you have a &str and the key type is String, you need to convert it to a string and take a reference to it, which is unergonomic and less performant. This does open the compiled code size to be larger due to added generics use, but I think it's worth exploring.

I think it would be also awesome if we added the entry API to these types, which would come with some overhead of having to cache all values (which I think is a thing to strive for anyway to avoid redundant storage reads), but it allows for some very clean usage.

If these data structures are modified to cache loaded values from storage, which would take some careful checking of the safety to make sure the API couldn't be misused, we can have APIs for these collections that more closely match std and also potentially save on gas usages from redundant reads.

I'll edit this comment with other ideas to list in the same form as your list, I just needed to add some additional context for this example because it's larger.

[matklad]

Right, there's also a minor point that UnorderedMap is unlikely to be the best name -- it feels like a carry over from C++, where this name was chosen due to backwards compatibility concerns: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2003/n1456.html

[evgenykuzyakov]

I'd also recommend replace AccountId from being a String to be struct(pub Vec<u8>), so we don't need to call from_utf8 when not necessary

[austinabell]

I'd also recommend replace AccountId from being a String to be struct(pub Vec<u8>), so we don't need to call from_utf8 when not necessary

Can the AccountId be non-utf8 bytes on-chain? Seems like you're sacrificing developer UX for slightly cleaner code internally? Ideally, we don't expose the internal value to make keeping semver easier.

[austinabell]

Okay, I've updated the original post by changing to checkboxes for completion, linking to PRs, and separating it into potential 4.0 upgrades and future updates.

Feel free for anyone to add anything or change where the items are if you think anything should/shouldn't be included

[austinabell]

Also as a point of discussion, possibly in this release, we can rename some of the VMContext attributes ref #14. This is currently a breaking change because all fields are publicly exposed (which is a nightmare to maintain), but can be easily phased out depending on if people have started to switch over to VMContextBuilder and don't modify the VMContext directly. Does anyone have thoughts?

Perhaps this can be in a future release, as it would be annoying to migrate for only a cosmetic change

[matklad]

Note that VMContext is a type which comes from nearcore, so we can't just rename the fields. Although it seems it also is amentable to the re-definition trick (together with VMConfig). I would't try to do renames just now: I think we shot for minimally disruptive release, and what just to untie our hands.

So:

• don't do renames
• but do re-exports and hide those behind #[cfg(not(target = wasm32))]
• audit code for more accidental re-exports. pub fn get_created_receipts() -> Vec<Receipt> feels suspicions. It's public, but there's nothing the user can do with Receipts.

[austinabell]

Added switching storage key type to the list of future improvements based on #402 (comment). I've kept in future changes because changing the trait return type can add additional breakages that might be unwanted. Alternatively or before then can just change the types which contain the keys (collections) to Box<[u8]> and just convert it before initializing.

Possibly could be added in the next release though, does anyone have thoughts?

[matklad]

Something which occurred to me yesterday: in env, we have functions like env::current_account_id or env::block_height. Another way to spell this would be making the functions associated methods of the corresponding types: AccountId::current, BlockHeight::current. Not entirely sure which design is better. I think I have a slight preference towards the status quo.