feat(json_types): implement FromStr for Base58CryptoHash and cleanup #398

austinabell · 2021-05-03T16:14:43Z

a similar change to #391 and also cleanup for readability and deserialize type for potential performance increase (Cow can avoid an allocation depending on the underlying deserialize buffer).

matklad

Hm, this got me thinking...

Do we perhaps want to introduce pub struct ParseCrypoHashError error, while we are adding this API anyway? There's one problem with Box<dyn Error> -- it's not trivially compatible with anyhow and such. That is, you can't ? one type-erased error into the other.

matklad · 2021-05-03T16:17:30Z

near-sdk/src/json_types/hash.rs

- let s = <String as serde::Deserialize>::deserialize(deserializer)?;
- s.try_into()
- .map_err(|err: Box<dyn std::error::Error>| serde::de::Error::custom(err.to_string()))
+ let s: Cow<'de, str> = Deserialize::deserialize(deserializer)?;


👍 nice optimization!

near-sdk/src/json_types/hash.rs

matklad · 2021-05-03T16:19:37Z

near-sdk/src/json_types/hash.rs

+}
+
+impl FromStr for Base58CryptoHash {
+ type Err = Box<dyn std::error::Error>;


If this were a general-purpose probject, I'd write

Suggested change

type Err = Box<dyn std::error::Error>;

type Err = Box<dyn std::error::Error + Send + Sync>;

but I guess for us it doesn't really make sense

I didn't want to open up to a breaking change, but I've created an alias to make this error type easier to switch in the future

This particular impl is new, so, for this impl, we can change the error type, as long as it is convertable to original one via ?. In particular, changing this (but only this) type to ParseCryptoHashError would work.

which ParseCryptoHashError are you referring to? The box std::error with send/sync or are you referring to something specific/ generating a new error type?

Sorry for being confusing here. I mean we can do the following, which should is backwards compatible, but, at the same time, is forwards compatible with a nice API of a hypothetical ideal sdk.

impl TryFrom<&str> for Base58CryptoHash { type Error = Box<dyn std::error::Error>; type Error = Error; fn try_from(value: &str) -> Result<Self, Self::Error> { let res = value.parse()?; // `?` converts ParseCryptoHashError to Box<dyn std::error::Error> Ok(res) } } #[non_exhaustive] pub struct ParseCryptoHashError; impl std::error::Error for ParseCryptoHashError { ... } impl std::str::FromStr for Base58CryptoHash { // Ok to use new type here, as this is a new impl, so we don't care about backcompat. type Err = ParseCryptoHashError; fn from_str(value: &str) -> Result<Self, ParseCryptoHashError> { ... } }

Yeah, the only reason I'm hesitant to add this is that it's unclear what the exact Error will be in the future. Using a concrete type like this enforces we keep that type (and trait bounds) in this ideal future API.

I'm happy to make the change and manually implement std::error if someone gives me the go-ahead with a thumbs up on this comment

is that it's unclear what the exact Error will be in the future

I think it's probably good to keep errors for well-isolated parts of code specific. IE, even we have something like type NearError = ... in the future, it would still make sense to use ParseCryptoHashError here.

I don't think that'll affect the size of the compiled contract (the type is ZST, the implementation is trivial and inlinable, etc), though my confidence level here is not 100%.

austinabell · 2021-05-03T16:29:20Z

Hm, this got me thinking...

Do we perhaps want to introduce pub struct ParseCrypoHashError error, while we are adding this API anyway? There's one problem with Box<dyn Error> -- it's not trivially compatible with anyhow and such. That is, you can't ? one type-erased error into the other.

I'm glad you brought it up. I was going to open an issue around changing this as the standard, as this error type is used everywhere. I was going to ask if this would be an acceptable change but was not in a rush because I didn't know if the base type was used to minimize compilation size.

evgenykuzyakov · 2021-05-03T19:20:26Z

near-sdk/src/json_types/hash.rs

@@ -1,6 +1,10 @@
 use crate::CryptoHash;
 use borsh::{BorshDeserialize, BorshSerialize};
-use std::convert::{TryFrom, TryInto};
+use serde::{de, ser, Deserialize};
+use std::borrow::Cow;


We should try to rebuild some contract that uses Base58CryptoHash with the contract builder, to compare the compilation size with using Cow. I don't expect anything dramatic, but just to be careful

if you give me a workflow or point me in the direction of one that does, I can do that. I'd be curious to see if it actually increases the compiled code (and to what scale).

Not sure what's the existing workflow, but I'd suggest adding a smoke test for this.

Something stupid like:

#[test] fn test_we_are_smol() { std::process::Command::new("cargo") .current_dir("examples/locakable-fungible-token") .env("RUSTFLAGS", "--link-args=-s") .args(&["build", "--release", "--target", "wasm"]) .status().unwrap() let wasm = std::fs::read("examples/locakable-fungible-token/target/wasm").unwrap(); assert!(100k <= wasm && wasm <= 300k) }

You can use Base58CryptoHash in examples/test-contract and compile it before the change and after the change using contract builder https://github.com/near/near-sdk-rs/tree/master/contact-builder and compare binary sizes

austinabell · 2021-05-06T18:51:25Z

before this and #391 are pulled in, should probably come to consensus or I just implement a concrete error type from the new parse method

…o b58hash_fstr

mikedotexe · 2021-05-11T16:43:06Z

near-sdk/src/json_types/hash.rs

 let mut crypto_hash: CryptoHash = CryptoHash::default();
 let size = bs58::decode(value).into(&mut crypto_hash)?;
 if size != std::mem::size_of::<CryptoHash>() {
- return Err("Invalid length of the crypto hash (32)".into());
+ return Err(ParseCryptoHashError::InvalidLength(size));


mikedotexe · 2021-05-11T16:45:24Z

near-sdk/src/json_types/hash.rs

 }
 Ok(Self(crypto_hash))
 }
 }
+
+#[non_exhaustive]


Interesting, I'm making a mental note of this as it may be a macro we'd think about adding when we use Enums for contract upgradability. Like:
https://nomicon.io/ChainSpec/Upgradability.html#versioned-data-structures

Yeah it's super handy when handling releases as it opens a lot of changes to not break semver, but can be harder to track the additions on new releases externally

pedantry police: #[non_exhaustive] is an attribute. Some attributes are macros (like derives or #[near_bindgen]), but this one isn't, it's just some meta info for the compiler.

matklad · 2021-05-11T17:28:24Z

near-sdk/src/json_types/hash.rs

+#[derive(Debug)]
+pub enum ParseCryptoHashError {
+ InvalidLength(usize),
+ Base58(B58Error),


Note that this makes B58Error a public API, so if we chose to switch to a different library to implement this (or even do a major upgrade of Base58), then we'd have to do a major semver bump. I'd personally would try to be concervative with our errors and a) get maximum room for future extensibility b) without exposing internal implementation details. So, something like this:

pub struct ParseCryptoHashError { kind: ParseCryptoHashErrorKind } enum ParseCryptoHashErrorKind { InvalidLength(usize), Base58(B58Error), }

How about switching the B58 Error to be a String or something? The functionality that would be nice to keep is being able to match the error type, and this can't happen if the kind is private.

Or I can just switch to keep the variants private in for this (and the other usage of b58 errors), let me know if you think that's better. I did overlook exposing the b58 error type

Do people need to match over the error though? My gut feeling is that people generally need only a Display out of this error.

For error classification of foundational libraries, the ErrorKind pattern, as used by std::io::Error is the best -- you keep the error opaque, but there's a .kind() fildless enum which allows for matching .

yeah probably best to be safe for now, I'll keep the types private for now, and can just expose later if/when solidified

austinabell added 2 commits May 3, 2021 12:12

feat: implement FromStr for Base58CryptoHash and cleanup

29204d6

chore: cleanup import

0051773

austinabell requested review from chadoh, evgenykuzyakov and mikedotexe as code owners May 3, 2021 16:14

matklad reviewed May 3, 2021

View reviewed changes

austinabell added 3 commits May 3, 2021 12:32

chore: create alias for error type to easily switch in future

fdeaef7

chore: cleanup namespacing

fb9487f

fix: fmt

d154871

austinabell mentioned this pull request May 3, 2021

Tracking issue for error standard in SDK #399

Closed

evgenykuzyakov reviewed May 3, 2021

View reviewed changes

evgenykuzyakov approved these changes May 6, 2021

View reviewed changes

Merge branch 'master' into b58hash_fstr

96fbc2d

austinabell added 3 commits May 10, 2021 09:59

switch new impl to concrete type

7dbc66d

Merge branch 'master' of github.com:near/near-sdk-rs into b58hash_fstr

db3d48b

Merge branch 'b58hash_fstr' of github.com:austinabell/near-sdk-rs int…

65633aa

…o b58hash_fstr

austinabell changed the title ~~feat: implement FromStr for Base58CryptoHash and cleanup~~ feat(json_types): implement FromStr for Base58CryptoHash and cleanup May 10, 2021

Merge branch 'master' into b58hash_fstr

a38294f

mikedotexe reviewed May 11, 2021

View reviewed changes

mikedotexe approved these changes May 11, 2021

View reviewed changes

mikedotexe merged commit cc73d55 into near:master May 11, 2021

matklad reviewed May 11, 2021

View reviewed changes

austinabell mentioned this pull request May 11, 2021

chore: remove public visibility of b58 error variant #414

Merged

austinabell mentioned this pull request May 20, 2021

List of odd APIs #387

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(json_types): implement FromStr for Base58CryptoHash and cleanup #398

feat(json_types): implement FromStr for Base58CryptoHash and cleanup #398

austinabell commented May 3, 2021

matklad left a comment

matklad May 3, 2021

matklad May 3, 2021

austinabell May 3, 2021

matklad May 3, 2021

austinabell May 3, 2021

matklad May 3, 2021

austinabell May 3, 2021

matklad May 4, 2021

austinabell commented May 3, 2021

evgenykuzyakov May 3, 2021

austinabell May 3, 2021

matklad May 4, 2021

evgenykuzyakov May 6, 2021 •

edited

Loading

austinabell commented May 6, 2021

mikedotexe May 11, 2021

mikedotexe May 11, 2021

austinabell May 11, 2021

matklad May 11, 2021

matklad May 11, 2021

austinabell May 11, 2021

austinabell May 11, 2021

matklad May 11, 2021

austinabell May 11, 2021

	type Err = Box<dyn std::error::Error>;
	type Err = Box<dyn std::error::Error + Send + Sync>;

feat(json_types): implement FromStr for Base58CryptoHash and cleanup #398

feat(json_types): implement FromStr for Base58CryptoHash and cleanup #398

Conversation

austinabell commented May 3, 2021

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell commented May 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgenykuzyakov May 6, 2021 • edited Loading

Choose a reason for hiding this comment

austinabell commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgenykuzyakov May 6, 2021 •

edited

Loading