near / borsh-rs

Rust implementation of Binary Object Representation Serializer for Hashing
https://borsh.io/
Apache License 2.0
287 stars 62 forks source link

Old NEAR contracts won't compile with the new `borsh` re-exported from `near-sdk-rs` ("Could not find `borsh`") #277

Closed fadeevab closed 6 months ago

fadeevab commented 6 months ago

I hit the issue with the new near-sdk-rs: https://github.com/near/near-sdk-rs/issues/1129

I tried to compile my project with near-sdk-rs 5.0.0-alpha.1 because of this issue https://github.com/near/near-sdk-rs/issues/1119, however, build fails with the errors:

error: proc-macro derive panicked
 --> src/main.rs
  |
6 | #[derive(BorshSerialize, BorshDeserialize)]
  |          ^^^^^^^^^^^^^^
  |
  = help: message: called `Result::unwrap()` on an `Err` value: Could not find `borsh` in `dependencies` or `dev-dependencies` in `/home/xxxx/yyyyy/Cargo.toml`!

BEFORE, a simple import of near_sdk::borsh::self was enough to make derive macro work. NOW, either a direct borsh import is needed (thus, re-export from near-sdk becomes useless), or #[borsh(crate = "near_sdk::borsh")] is necessary for every structure (as suggested by @frol, https://github.com/near/near-sdk-rs/issues/1129#issuecomment-1889328251)

frol commented 6 months ago

This new behavior is by design and serde does the same.

fadeevab commented 6 months ago

@frol in this case, borsh re-export loses its sense, because using the borsh crate directly will be more convenient. I thought that re-export had to make things more convenient, not more complicated. I have 20 structures, I don't wanna add annotations to each one.

frol commented 5 months ago

@fadeevab Re-export also keeps things consistent. Ideally, I don't want to derive neither serde nor borsh manually, so I at the back of my mind I am thinking how to achieve that without introducing too much magic to it.

fadeevab commented 5 months ago

@frol :) One quick workaround came to my mind: falling back to near_sdk::borsh reexport if near_sdk has found - somewhere around these lines: https://github.com/near/borsh-rs/blob/19e9feee24749cbcb3226e09bf0df03059ee02a0/borsh-derive/src/internals/cratename.rs#L21

Little bit dirty :) However, intuitively, if I already use near_sdk::borsh::BorshDeserialize, I expect that #derive just works... Also, another justification to do so: borsh is already under the near (GitHub) account and ecosystem.

All that is not critical of course.

frol commented 5 months ago

@fadeevab Given that there are more than just borsh that needs to be configured https://github.com/near/near-sdk-rs/issues/1142, I don't think this change is worth it.