-
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
replace strict typed casting with deserialization #74
Conversation
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, this has fewer problems than our current approach, but it still depends on Rust ABI not changing between the point when you compiled cargo-near
and when you are invoking it (e.g. you used rustc 1.61 to compile cargo-near
, but now you are using rustc 1.64 to compile your smart contract).
From what I understand Rust ABI is not stable, so I would not be comfortable with depending on it not changing, unless someone has more insight on this topic. CC @austinabell
But thinking about this a bit more, couldn't we avoid Rust ABI entirely and load this with |
@itegulov, yeah I came to the same conclusion, but to be safe, we can replace |
Hmm, weird. Have you tried it? I am getting an empty file instead of ABI. |
Yeah, it works. Although, for posterity, the empty file issue was fixed in #75. |
This patch requires a version of CI fails for this reason. https://github.com/near/cargo-near/actions/runs/3281321184/jobs/5403176360#step:4:1081 |
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.
This looks good to me, but lets wait for the SDK change to come in first
@itegulov, this is ready. What do you think? Can you test it out? |
@miraclx hmm weird it is not working for me. Tried our classic |
Have you tried cleaning your target folder? Maybe cargo is returning an artifact from previous builds? In the meantime, I'll spin up a Linux machine and see if I can reproduce this. |
@miraclx yep, sorry, must have had some leftover artifacts indeed. Works fine now! |
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.
LGTM. Don't forget to update the git rev to the master commit though once the SDK PR is merged.
You got it, chief! |
Fixes #61, alternative to #73, depends on a small patch to SDK – near/near-sdk-rs#939.