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

feat: MVP for generating ext contract traits #1

Merged
merged 12 commits into from
Aug 25, 2022
Merged

feat: MVP for generating ext contract traits #1

merged 12 commits into from
Aug 25, 2022

Conversation

itegulov
Copy link
Contributor

@itegulov itegulov commented Jul 12, 2022

Supporting both macro & generation APIs seems to be a low-effort endeavor, so I am thinking to do just that. The only major downside I see is that having two ways of doing the same thing tends to cause confusion, but I feel like if we just stick to the macro in most of the examples/docs (just because it is less verbose) and state that there is an alternative way to do this via autogeneration we might be good. What do you guys think?

@itegulov itegulov marked this pull request as ready for review July 18, 2022 11:40
@itegulov itegulov requested review from austinabell and miraclx July 18, 2022 11:40
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

quick scan, I'll go more in depth tomorrow

PathBuf::from(abi_path.as_ref())
};

let abi_json = std::fs::read_to_string(&abi_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that embedding fs reads into the functionality isn't ideal because it won't be valid for every use case. Having this operate on already loaded bytes/string feels like a cleaner way to go where the interface expects &str (or &[u8] if we aren't confident this will always be json) which allows using something like https://doc.rust-lang.org/std/macro.include_str.html to avoid sync fs interactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my first intuition as well, but unfortunately macro invocations are evaluated from outside to inside. So this means that near_abi_ext!(include_str!("src/adder.json")) evaluates near_abi_ext! first and it fails because include_str!(...) is not a string literal. Unless I am missing something here and you know of an alternative way to do this?

near-sdk-abi/src/lib.rs Outdated Show resolved Hide resolved
near-sdk-abi/src/lib.rs Outdated Show resolved Hide resolved
@austinabell austinabell mentioned this pull request Jul 25, 2022
@coleFD
Copy link

coleFD commented Aug 2, 2022

How far along with this project are you guys and what is the desired outcome (what will the abi look like and how will they be generated)? I am also working on an abi generator for rust contracts and want to make sure there is no overlap here. I am taking a different approach but I have a parser, crate dependency graph, and filesystem map built. The last step is to build the abi with the said structures so not much work left. I also saw the generated client repos which is what I planned to work on next.

If you all are down, we could hop on a call as well to discuss

@austinabell
Copy link
Contributor

How far along with this project are you guys and what is the desired outcome (what will the abi look like and how will they be generated)? I am also working on an abi generator for rust contracts and want to make sure there is no overlap here. I am taking a different approach but I have a parser, crate dependency graph, and filesystem map built. The last step is to build the abi with the said structures so not much work left. I also saw the generated client repos which is what I planned to work on next.

If you all are down, we could hop on a call as well to discuss

Basically, for the first version, we have everything connected from SDK to some clients (this, https://github.com/near/near-abi-client-rs, https://github.com/near/near-abi-client-js) through https://github.com/near/cargo-near.

Can see an example ABI here https://github.com/near/near-sdk-rs/blob/master/examples/abi/res/abi.json

Things still aren't production ready or stabilized yet, but the flow should stay roughly the same

pub mod_name: Option<String>,
}

impl AbiFile {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add builder patterns to be able to chain the initialization? Intuition is that overriding the contract/module name might be a bit awkward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed a little awkward to me to have 4 different combinations of (opt_contract_name, opt_mod_name) as opposed to just 2 like in near-abi-client-rs.

My idea was to do something like Generator::new(...).file(AbiFIle { path: ..., contract_name: None, mod_name: Some("ext_cool_contract") }. What would be a better alternative?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's fair, just indicating that this might be difficult to maintain in the future because any addition of fields or changing of types will break the API

Comment on lines +17 to +23
pub path: PathBuf,
/// Contract name to be used for the resulting trait name.
/// If missing will try to pull the name from ABI metadata and use `Ext<ContractName>`.
pub contract_name: Option<String>,
/// mod name to be used for the resulting ext mod.
/// If missing will be derived by applying snake case to the contract name, e.g. ext_status_message.
pub mod_name: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

why do these need to be publicly exposed?

Copy link
Contributor Author

@itegulov itegulov Aug 3, 2022

Choose a reason for hiding this comment

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

Replied in the other thread (needed for constructing AbiFile struct)

use near_sdk_abi_macros::near_abi_ext;
use serde::{Deserialize, Serialize};

near_abi_ext! { mod ext_adder trait Adder for "src/adder.json" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: the trait name is only used when the module isn't included in the attribute. We could remove trait Adder from this codegen and just derive it if it isn't included.

I think the functionality should be kept because the trait generation could be useful if someone wanted to implement based on an ABI. Could consider having both optional where at least one is specified? Just sharing thoughts

@itegulov
Copy link
Contributor Author

itegulov commented Aug 3, 2022

How far along with this project are you guys and what is the desired outcome (what will the abi look like and how will they be generated)? I am also working on an abi generator for rust contracts and want to make sure there is no overlap here. I am taking a different approach but I have a parser, crate dependency graph, and filesystem map built. The last step is to build the abi with the said structures so not much work left. I also saw the generated client repos which is what I planned to work on next.

If you all are down, we could hop on a call as well to discuss

@coleFD Curious to hear more about your approach! Do you mind opening a discussion in https://github.com/near/cargo-near/discussions and share some details?

@coleFD
Copy link

coleFD commented Aug 3, 2022

How far along with this project are you guys and what is the desired outcome (what will the abi look like and how will they be generated)? I am also working on an abi generator for rust contracts and want to make sure there is no overlap here. I am taking a different approach but I have a parser, crate dependency graph, and filesystem map built. The last step is to build the abi with the said structures so not much work left. I also saw the generated client repos which is what I planned to work on next.
If you all are down, we could hop on a call as well to discuss

@coleFD Curious to hear more about your approach! Do you mind opening a discussion in https://github.com/near/cargo-near/discussions and share some details?

https://github.com/near/cargo-near/discussions/18

@itegulov itegulov merged commit 1a36e16 into main Aug 25, 2022
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