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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
May 11, 2021
Merged

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

Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
29204d6
feat: implement FromStr for Base58CryptoHash and cleanup
austinabell May 3, 2021
0051773
chore: cleanup import
austinabell May 3, 2021
fdeaef7
chore: create alias for error type to easily switch in future
austinabell May 3, 2021
fb9487f
chore: cleanup namespacing
austinabell May 3, 2021
d154871
fix: fmt
austinabell May 3, 2021
96fbc2d
Merge branch 'master' into b58hash_fstr
austinabell May 6, 2021
7dbc66d
switch new impl to concrete type
austinabell May 10, 2021
db3d48b
Merge branch 'master' of github.com:near/near-sdk-rs into b58hash_fstr
austinabell May 10, 2021
65633aa
Merge branch 'b58hash_fstr' of github.com:austinabell/near-sdk-rs int…
austinabell May 10, 2021
a38294f
Merge branch 'master' into b58hash_fstr
mikedotexe May 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions near-sdk/src/json_types/hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}

Copy link
Contributor

@evgenykuzyakov evgenykuzyakov May 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

use std::convert::TryFrom;

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

#[derive(
Debug, Copy, Clone, PartialEq, PartialOrd, Ord, Eq, BorshDeserialize, BorshSerialize, Default,
Expand All @@ -19,26 +23,22 @@ impl From<CryptoHash> for Base58CryptoHash {
}
}

impl serde::Serialize for Base58CryptoHash {
fn serialize<S>(
&self,
serializer: S,
) -> Result<<S as serde::Serializer>::Ok, <S as serde::Serializer>::Error>
impl ser::Serialize for Base58CryptoHash {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
S: ser::Serializer,
{
serializer.serialize_str(&String::from(self))
}
}

impl<'de> serde::Deserialize<'de> for Base58CryptoHash {
fn deserialize<D>(deserializer: D) -> Result<Self, <D as serde::Deserializer<'de>>::Error>
impl<'de> de::Deserialize<'de> for Base58CryptoHash {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
D: de::Deserializer<'de>,
{
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)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice optimization!

s.parse::<Self>().map_err(|err| de::Error::custom(err.to_string()))
}
}

Expand All @@ -49,17 +49,25 @@ impl From<&Base58CryptoHash> for String {
}

impl TryFrom<String> for Base58CryptoHash {
type Error = Box<dyn std::error::Error>;
type Error = Error;

fn try_from(value: String) -> Result<Self, Self::Error> {
Self::try_from(value.as_str())
}
}

impl TryFrom<&str> for Base58CryptoHash {
type Error = Box<dyn std::error::Error>;
type Error = Error;

fn try_from(value: &str) -> Result<Self, Self::Error> {
value.parse()
}
}

impl std::str::FromStr for Base58CryptoHash {
type Err = Error;

fn from_str(value: &str) -> Result<Self, Self::Err> {
let mut crypto_hash: CryptoHash = CryptoHash::default();
let size = bs58::decode(value).into(&mut crypto_hash)?;
if size != std::mem::size_of::<CryptoHash>() {
Expand Down