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

Prepare for stability (1.0 release) #51

Closed
23 of 24 tasks
matklad opened this issue Dec 16, 2021 · 27 comments
Closed
23 of 24 tasks

Prepare for stability (1.0 release) #51

matklad opened this issue Dec 16, 2021 · 27 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@matklad
Copy link
Contributor

matklad commented Dec 16, 2021

It feels like the ecosystem is big enough that the last breaking release, 1.0 is due. I don't think the current APIs are good enough for 1.0 release though, so I expect we need at least one more 0.x release with substantional breakage. This issue collects all know future-compatibility problems, but isn't intended to be full stabilization check-list just yet.

This was referenced Dec 16, 2021
@frol frol added enhancement New feature or request help wanted Extra attention is needed labels May 25, 2023
@frol frol mentioned this issue May 30, 2023
1 task
@iho
Copy link
Contributor

iho commented Jun 6, 2023

unsplit borsh-derive-internal

error: `proc-macro` crate types currently cannot export any items other than functions tagged with `#[proc_macro]`, `#[proc_macro_derive]`, or `#[proc_macro_attribute]`

I got this error because we export macroses and some functions. To do that we need to have crates.

@frol
Copy link
Collaborator

frol commented Jun 7, 2023

@iho Re: borsh-derive-internal

The question is whether we need to export those methods at all. I see no need in that, and serde went that path as well and 5 years ago it gave them 7% better compile time: serde-rs/serde@3859f58

@frol frol changed the title Prepare for stability Prepare for stability (1.0 release) Jun 19, 2023
@iho
Copy link
Contributor

iho commented Jun 19, 2023

I am going to work on

  • unsplit borsh-derive-internal
  • cleanup derive syntax to follow serde -- borsh(skip) rather bosh_skip

@frol
Copy link
Collaborator

frol commented Jun 19, 2023

@iho Re: unsplit borsh-derive-internal

I checked serde-derive-internals crate dependents and realized that they actually have quite a few dependencies there, so borsh-derive-internals might be not that useless after all, so before we merge the "unsplit" work, let's measure what we actually gain from it. If we see <5s compilation time improvement, I vote to keep it as is (two separate crates)

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 19, 2023

taking the following sub-issue for trial and error and resolution

  • Hide maybe_std module

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 19, 2023

taking the following sub-issue for trial and error and resolution

  • extract hashbrown dependency under a feature (as per clarification)

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 20, 2023

the following is covered in scope of #155

  • remove const-generic feature (it does nothing these days, and was only left to avoid a breaking change)

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 20, 2023

taking the following sub-issue for trial, error and resolution:

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 21, 2023

The following is (partially) covered in scope of #155 . nostd_io in integration test.
The completeness of coverage remains unclear atm.

  • add top-level io module

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 28, 2023

taking following sub-issue in scope of #46

  • Hide schema behind an schema cargo feature

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 29, 2023

taking this sub-issue:

  • hide derive behind an optional feature-flag

@frol
Copy link
Collaborator

frol commented Jun 29, 2023

@matklad Could you, please, remind what you had in mind when added this item?

  • Introduce #[doc(hidden)] pub __rt module with macro runtime for use by derives

It seems that the only macro runtime things are “maybestd” and the public traits, aren’t they? Given that we already renamed and hid “maybestd” to “__maybestd”, it seems that there is no further action items there worth introducing the module.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 29, 2023

taking this issue

  • clarify bounds on Rc/Arc impls we use something significantly funkier than what serde is doing

@matklad
Copy link
Contributor Author

matklad commented Jun 29, 2023

It seems that the only macro runtime things are “maybestd” and the public traits, aren’t they? Given that we already renamed and hid “maybestd” to “__maybestd”, it seems that there is no further action items there worth introducing the module.

Yup, if it’s just __maybestd, no further action is required. I would still maybe prefer to call this __runtime module, so that, if in the future we need to use any new name from macro, we can use a normal name and put it into __runtime module, rather than adding a different __ name to the top-level. Looking at serde and anyhow __private is probably a more standard name here than __runtime:

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jul 3, 2023

taking this sub-issue

  • Introduce #[doc(hidden)] pub __private module with macro runtime for use by derives

@iho
Copy link
Contributor

iho commented Jul 31, 2023

error: cannot find derive macro `BorshSerialize` in this scope
error[E0433]: failed to resolve: could not find `BorshDeserialize` in `borsh`

If you got one of those two errors, but your import is correct it means that you forgot to enable derive feature for borsh crate in your Cargo.toml.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Aug 14, 2023

taking following sub-issue:

  • test alpha release on near-sdk-rs and capture migration guide steps

@dj8yfo
Copy link
Collaborator

dj8yfo commented Aug 15, 2023

near-contract-standards v4.1.1 
├── near-sdk v4.1.1 
│   ├── borsh v1.0.0-alpha.2
│   ├── near-abi v0.3.0
│   │   ├── borsh v0.9.3
│   ├── near-crypto v0.17.0
│   │   ├── borsh v0.10.3
│   │   ├── near-account-id v0.17.0
│   │   │   ├── borsh v0.10.3 (*)
│   │   ├── near-config-utils v0.17.0
│   │   ├── near-stdx v0.17.0
│   ├── near-primitives v0.17.0
│   │   ├── borsh v0.10.3 (*)
│   │   ├── near-crypto v0.17.0 (*)
│   │   ├── near-fmt v0.17.0
│   │   │   └── near-primitives-core v0.17.0
│   │   │       ├── borsh v0.10.3 (*)
│   │   │       ├── near-account-id v0.17.0 (*)
│   │   ├── near-primitives-core v0.17.0 (*)
│   │   ├── near-rpc-error-macro v0.17.0 (proc-macro)
│   │   ├── near-stdx v0.17.0
│   │   ├── near-vm-errors v0.17.0
│   │   │   ├── borsh v0.10.3 (*)
│   │   │   ├── near-account-id v0.17.0 (*)
│   │   │   ├── near-rpc-error-macro v0.17.0 (proc-macro) (*)
│   ├── near-primitives-core v0.17.0 (*)
│   ├── near-sdk-macros v4.1.1 (proc-macro) 
│   ├── near-sys v0.2.0 
│   ├── near-vm-logic v0.17.0
│   │   ├── borsh v0.10.3 (*)
│   │   ├── near-account-id v0.17.0 (*)
│   │   ├── near-crypto v0.17.0 (*)
│   │   ├── near-fmt v0.17.0 (*)
│   │   ├── near-o11y v0.17.0
│   │   │   ├── near-crypto v0.17.0 (*)
│   │   │   ├── near-primitives-core v0.17.0 (*)
│   │   ├── near-primitives v0.17.0 (*)
│   │   ├── near-primitives-core v0.17.0 (*)
│   │   ├── near-stdx v0.17.0
│   │   ├── near-vm-errors v0.17.0 (*)
│   │   └── zeropool-bn v0.5.11
│   │       ├── borsh v0.9.3 (*)

near-sdk v4.1.1  (*)

near-sdk-macros v4.1.1 (proc-macro)  (*)

near-sys v0.2.0 

as dependencies are alined as follows: borsh -> nearcore -> near-sdk-rs,
instead of borsh -> near-sdk-rs -> nearcore
taking the following sub-issue first

  • test alpha release on nearcore and capture migration guide steps, create a draft PR (only merge after 1.0.0 release)

@dj8yfo
Copy link
Collaborator

dj8yfo commented Aug 18, 2023

taking following sub-issue for resolution:

  • test alpha release on near-sdk-rs and capture migration guide steps, create a draft PR (only merge after 1.0.0 release)

@matklad
Copy link
Contributor Author

matklad commented Aug 18, 2023

So the alpha is out and the API is mostly settled? I think I can allocate some time to take a look at what we've arrived at. No promises, so don't block on me, but I'll try to do API review before Monday!

@dj8yfo
Copy link
Collaborator

dj8yfo commented Aug 18, 2023

So the alpha is out and the API is mostly settled?

I hope so.
@matklad But don't hesitate to post a review, if you find any minor or serious flaws.

@matklad
Copy link
Contributor Author

matklad commented Aug 18, 2023

Notes from review (haven't look at the code for a while, so take as a weak advice as most!):

borsh-derive = { path = "../borsh-derive", version = "1.0.0-alpha.3", optional = true }

You want version = "=1.0.0-alpha.3" version most likely, to ensure that derives and the main crate are in sync.

hashbrown = { version = ">=0.11,<0.14", optional = true }

probably makes sense to just switch to hashbrown=0.14. Or maybe even completely get rid of it? There's probably some historical reason why we need it, but, fundamentally, we shouldn't be neededing.

* **std** -
When enabled, this will cause `borsh` to use the standard library. Currently,
disabling this feature will result in building the crate in `no_std` environment.

This should probably say something about nostd_io traits as that's the main unusual thing you get with no std borsh.

https://github.com/near/borsh-rs/blob/master/borsh/src/lib.rs#L39

It's a bit surprising that that's not the default. Probably makes sense to explain why we need this feature:

If this feature is not enabled, it is possible that two different byte slices could deserialize into the same object

* **hash_collections** -
This is a feature alias, set up in `build.rs` to be equivalent to (**std** OR **hashbrown**).
Gates implementation of [BorshSerialize](crate::ser::BorshSerialize), [BorshDeserialize](crate::de::BorshDeserialize)
and [BorshSchema](crate::schema::BorshSchema)
for [HashMap](std::collections::HashMap)/[HashSet](std::collections::HashSet).
* **derive_schema** -
This is a feature alias, set up in `build.rs` to be equivalent to (**derive** AND **schema**).

This is implementation detail, so it shouldn't go into user-facing docs. Additionally, if having feature aliases is the only reason to have build.rs, I'd probably recomend removing those --- build.rs slows down compilation somewhat, because build script needs to get compiled, linked and run even before the crate could start compiling.

pub mod schema_helpers;

_helpers is an odd name for publicly available API.

"feature \"schema\" depends on \"derive\" feature in its implementation; enable it too.."

Duplicate full stop.

let mut res = schema.try_to_vec()?;
res.extend(value.try_to_vec()?);

Could save an allocation of intermediate vector here.

variants: Vec<(VariantName, Declaration)>,

Here and in structs, we don't actually enforce that names are unique. So potentially someone can hand-craft schemas with duplicate names and cause some mischief. Not sure if we need to worry about this. Similarly, we don't enforce that all declarations have corresponding definitions.

fn add_definition(

Feels like an odd method for a trait. It makes no sense to override this, so it probably should be a free-standing function somewhere. Also, I belive implementatin can be simplifid to

let prev = defitnions.insert(declaration, definiton);
assert!(prev.is_none())

fn schema_container() -> BorshSchemaContainer {

Similarly how we add top-level borsh::from_slice, this shouldn't be a trait method, just a top-level borsh::shema_of::<T> function.

fn try_to_vec(&self) -> Result<Vec<u8>> {

Given borsh::to_vec we want to depricate or remove this method.

assert!(

we return InvalidData when trying to serialize too long slices. So we probably shold InvalidData here as well.

if size_of::<T>() == 0 {
return Err(Error::new(
ErrorKind::InvalidData,
"Vectors of zero-sized types are not allowed due to deny-of-service concerns on deserialization.",
));

I am not sure, but perhaps Rust is advanced enough that its possible to leveate this to a compile-time error via some const-fn trickery?

vec.sort_by(|(a, _), (b, _)| a.partial_cmp(b).unwrap());

Similarly, might want to InvalidData rather than unwrap here?

let mut vec = self.iter().collect::<Vec<_>>();

this should be consistent with Vec wrt handling zero-sized types

pub trait EnumExt: BorshDeserialize {

does this want to be in the __private module perhaps?

@dj8yfo
Copy link
Collaborator

dj8yfo commented Aug 21, 2023

vec.sort_by(|(a, _), (b, _)| a.partial_cmp(b).unwrap());

I've also noticed this one. I've skipped it so far for 2 reasons:

  1. One needs to constructs an Eq + PartialOrd<Self> type in order to be able to trigger this panic, which feels kind of artificial.
  2. I haven't found a fallible sort helper in standard lib.

Edit: probably the best fix for this is restricting the bound on type K to Ord, as K: PartialOrd + partial_cmp(b).unwrap() is equivalent to using subset of K, which is Ord.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Sep 9, 2023

taking this subissue

  • rename schema cargo feature flag to unstable__schema

@maxammann
Copy link

I marked the 1.0.0 as fixed in Rustsec: rustsec/advisory-db#1794

@frol
Copy link
Collaborator

frol commented Oct 3, 2023

@matklad Could you, please, take one more last look here? We are about to cut 1.0.0!

@matklad
Copy link
Contributor Author

matklad commented Oct 10, 2023

Sorry, was taking the previous week off of GitHub, but I trust you all to do a good job here!

Thanks for persevering, 1.0 for borsh is quite important!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants