refactor: move type aliases from deps #415
Conversation
There was a problem hiding this comment.
Looks good for me as is, I'd probably do wasm32 exclusion separately.
I suggest compiling https://github.com/near/core-contracts with this PR, to smoke test that this is as compatible as we think.
| /// Account identifier. Provides access to user's state. | ||
| pub type AccountId = String; | ||
| /// Hash used by a struct implementing the Merkle tree. | ||
| pub type MerkleHash = CryptoHash; |
There was a problem hiding this comment.
So this is going to be a tough one. SDK defines a simple CryptoHash
But this MerkleHash is also public :-(
Can we quickly grep existing contracts to see if they use MerkleHash? Just to understand what leeway we have with changing that?
Although it seems that porting primitives-code CryptoHash and Digest won't be hard, just annoying.
There was a problem hiding this comment.
Yeah, I don't see a need for it to be exposed, but I was just trying to keep the API exactly as it was for now. I'll try to look to find any examples of using MerkleHash, but I doubt there would be any references to it through the SDK.
There was a problem hiding this comment.
but I doubt there would be any references to it through the SDK.
I meant grepping extenral contracts, like near/core-contracts.
There was a problem hiding this comment.
Also headsup here -- CrypoHash definition in nearcore master branch is slightly different from the one used here.
There was a problem hiding this comment.
but I doubt there would be any references to it through the SDK.
I meant grepping extenral contracts, like near/core-contracts.
Yeah, what I meant by that is that I doubt people reference that type through the SDK because there is no connection with the SDK.
Also headsup here -- CrypoHash definition in nearcore master branch is slightly different from the one used here.
Yeah this is an overridden type, there are a few of these, and I'm not quite sure why yet
There was a problem hiding this comment.
I don't think MerkleHash is used anywhere by the contracts. At least I'm not aware of one
| @@ -0,0 +1,45 @@ | |||
| use near_primitives_core::hash::CryptoHash; | |||
|
|
|||
| pub use near_primitives_core::runtime::fees::RuntimeFeesConfig; | |||
There was a problem hiding this comment.
This should be restricted to non-wasm32 as well, right?
There was a problem hiding this comment.
yeah, was going to do in follow up PR, but can make that change now (was being cautious to avoid removing if needed)
There should be no change to the API from this PR. I will note that there is a
PromiseIndexin the two crates where it is au64innear_vm_typesandusizeinnear_primatives_corebut I kept the type alias that showed up in cargo docs (u64).cc @matklad, was this what you had in mind for the first part of this? I can probably exclude the primitives and vm_logic types for wasm env with these changes. I will do that now and link to this