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

ABI embedding #893

Merged
merged 47 commits into from
Aug 29, 2022
Merged

ABI embedding #893

merged 47 commits into from
Aug 29, 2022

Conversation

miraclx
Copy link
Contributor

@miraclx miraclx commented Aug 21, 2022

Based off #889.

Implements functionality for embedding ABI into the contract.

Related: near/cargo-near#31

@miraclx miraclx changed the title extract ABI primitives into near-abi-rs ABI embedding Aug 21, 2022
@itegulov
Copy link
Contributor

I have tried this flow and seems to work fine, great job @miraclx!

I have a couple of questions though:

  1. Any reason for the ABI view function to be named __contract_abi? I don't see why it should be "private"/"hidden", calling it manually (outside of dev console) I think is a totally valid use case.
  2. Should __contract_abi itself be a part of ABI? You can easily inject it into the ABI file, but I guess this also relates to the visibility status of this function.

@miraclx
Copy link
Contributor Author

miraclx commented Aug 24, 2022

Any reason for the ABI view function to be named __contract_abi?

No reason, that's a placeholder that I hoped would be descriptive enough. But we can discuss this. I'm open to changing it.

I don't see why it should be "private"/"hidden", calling it manually (outside of dev console) I think is a totally valid use case.

Hm, but It's not hidden though. Unless I misunderstand. You can call the function inspite of dev console. So far you know the __contract_abi method exists.

Should __contract_abi itself be a part of ABI?

Uhh, that's a good question. Well, the function signature is simple enough to not require being inspected at generation time, so it's technically possible to inject that post ABI generation. But would we have to add definitions for the return type? And have AbiRoot serialized into itself. Or we just define it as arbitrary bytes.

@itegulov
Copy link
Contributor

itegulov commented Aug 24, 2022

No reason, that's a placeholder that I hoped would be descriptive enough. But we can discuss this. I'm open to changing it.

Hm, but It's not hidden though. Unless I misunderstand. You can call the function inspite of dev console. So far you know the __contract_abi method exists.

I am specifically referring to the __ prefix, the contract_abi parts sounds good to me. You can of course call it, it just feels like we are discouraging it by choosing a name like this.

UPD: On second thought it does increase the chances of someone already having a function named contract_abi vs __contract_abi...

But would we have to add definitions for the return type? And have AbiRoot serialized into itself. Or we just define it as arbitrary bytes.

It would technically be impossible to describe ABI file inside itself (unless we exclude the JSON Schema part) and the usefulness of it would be questionable at best anyway. So yeah a bunch of bytes sounds good to me.

@miraclx
Copy link
Contributor Author

miraclx commented Aug 24, 2022

On second thought it does increase the chances of someone already having a function named contract_abi vs __contract_abi

Yeah, exactly. Was just working on returning a neat error when someone uses the __contract_abi method name in their contract. Regardless of whether they have the abi feature flag on. I think it's best to treat it as reserved much like https://doc.rust-lang.org/reference/keywords.html#reserved-keywords, but we can discuss this.

It would technically be impossible to describe ABI file inside itself

schemars actually has impl_json_schema feature flag, so maybe not technically “impossible”, but it would lead to a bloated ABI at the end of the day. https://docs.rs/schemars/latest/schemars/#feature-flags

@itegulov
Copy link
Contributor

schemars actually has impl_json_schema feature flag, so maybe not technically “impossible”, but it would lead to a bloated ABI at the end of the day. docs.rs/schemars/latest/schemars/#feature-flags

Well, unlike us, they don't then go and add these schemas to their codebase, making what they originally derived invalid :)

@miraclx
Copy link
Contributor Author

miraclx commented Aug 24, 2022

Was just working on returning a neat error when someone uses the __contract_abi method name in their contract. Regardless of whether they have the abi feature flag on. I think it's best to treat it as reserved much like doc.rust-lang.org/reference/keywords.html#reserved-keywords, but we can discuss this.

error: use of reserved contract method
  --> src/lib.rs:43:12
   |
43 |     pub fn __contract_abi(&self) -> &str {
   |            ^^^^^^^^^^^^^^

error: could not compile `abi` due to previous error

.to_compile_error(),
);
}
}
Copy link
Contributor

@itegulov itegulov Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but just noting that this would technically be a breaking change. Probably not big enough deal to warrant going into 5.x though.

Base automatically changed from miraclx/extract-lib-abi to master August 24, 2022 13:49
@miraclx miraclx changed the base branch from master to update-abi August 25, 2022 08:17
Base automatically changed from update-abi to master August 25, 2022 16:15
examples/abi/src/lib.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants