-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: enforce near-abi
version match
#70
Conversation
Will add a test for this once we release near-sdk 4.1.0-pre.4 with near-abi 0.2.0 |
let actual_abi_ver = &near_abi_dep.version; | ||
if expected_abi_ver.major != actual_abi_ver.major | ||
|| expected_abi_ver.minor != actual_abi_ver.minor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, should we be testing with the crate version? or the schema_version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why this check is needed in the first place is because ChunkedAbiEntry
returned from the dylib artifact has to conform to the ChunkedAbiEntry
used by cargo-near
, which depends on how Rust ABI views them. For example I highly doubt that Rust would allow you to interpret:
pub struct AbiMetadata {
pub name: Option<String>,
pub other: HashMap<String, String>,
}
and
pub struct AbiMetadata {
pub name: Option<String>,
pub version: Option<String>,
pub other: HashMap<String, String>,
}
as the same object even if they both can belong to the same schema_version
(extending metadata object does not bump the schema version obviously).
If we were to switch back to JSON-based ABI chunk generation, then we would be able only to check the schema_version
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I understand that, suggesting we could consider returning serde_json::Value
from the chunks, instead of the strict type. Then we can leave it up to the deserialization implementation to do the validation. Or what do you think?
But as-is works too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that could work. In general, do you know what invariants you can depend on when working with low-level Rust ABI? For example, is it reasonable to assume that serde_json::Value
is going to have the same ABI for all major.minor
releases? Also, would I have to re-compile cargo-near whenever I install a new version of rustc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, honestly, best path to avoid inconsistencies later down the line would probably be to return JSON strings. And manually handle deserialization wherein we can validate the schema_version
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I am thinking too right now. Any ideas on how to get schema_version
from a crate though? Can we add like a custom cargo metadata key or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, ChunkedAbiEntry
already includes schema_version
.
Which means we can only do the compatibility check after the Generating ABI
step.
EDIT: ... or rather, during..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I especially like that ChunkedAbiEntry
already does this compatibility check on deserialization.
So we get this for free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I will prepare an alternative PR then
Closed in favor of #74 |
Fixes #61
Just a dumb
MAJOR.MINOR
check for now, we can tweak it later if needed