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

Generate Solidity compatible metadata #1930

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

davidsemakula
Copy link
Contributor

@davidsemakula davidsemakula commented Feb 3, 2025

Summary

Closes #1070
Also closes use-ink/ink-alliance#13

  • [y] y/n | Does it introduce breaking changes?
  • [n] y/n | Is it dependent on the specific version of ink or pallet-contracts?

Adds option to generate Solidity compatible metadata files

Description

  • Adds a CLI arg cargo contract build --metadata <ink|solidity> for choosing the specification to use for metadata generation
  • Adds ability to generate Solidity compatible ABI and metadata files (i.e. <artifact_name>.abi and <artifact_name>.json)

Checklist before requesting a review

  • My code follows the style guidelines of this project
  • I have added an entry to CHANGELOG.md
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@davidsemakula davidsemakula force-pushed the solidity-compatible-metadata branch 3 times, most recently from 0ef07ac to 2aaf6ad Compare February 4, 2025 16:38
@davidsemakula davidsemakula marked this pull request as ready for review February 4, 2025 18:11
@davidsemakula davidsemakula force-pushed the solidity-compatible-metadata branch from 55fab68 to 00b6e27 Compare February 4, 2025 23:23
cmichi added a commit that referenced this pull request Feb 5, 2025
cmichi added a commit that referenced this pull request Feb 5, 2025
* Fix `clippy` warning

* Rename `encode::tests::incrementer` to prevent build conflict with `verify::tests::incrementer`
@davidsemakula davidsemakula force-pushed the solidity-compatible-metadata branch from 63b128b to b96c2e5 Compare February 5, 2025 14:12
@cmichi
Copy link
Collaborator

cmichi commented Feb 5, 2025

@davidsemakula I ran cargo contract build --metadata solidity against integration-tests/ in ink/master and for some contracts it fails because they use the newly introduced Solidity types (ink_primitves::{H160, H256, U256}.

E.g. here:

internal/lang-err/call-builder/Cargo.toml

ERROR: Solidity ABI incompatible type in arg `address` for message `call`: Type { path: Path { segments: ["primitive_types", "H160"] }, type_params: [], type_def: Composite(TypeDefComposite { fields: [Field { name: None, ty: UntrackedSymbol { id: 5, marker: PhantomData<fn() -> core::any::TypeId> }, type_name: Some("[u8; 20]"), docs: [] }] }), docs: [] }
internal/e2e-runtime-only-backend/Cargo.toml

ERROR: Solidity ABI incompatible type in message `get_contract_balance`: Type { path: Path { segments: ["primitive_types", "U256"] }, type_params: [], type_def: Composite(TypeDefComposite { fields: [Field { name: None, ty: UntrackedSymbol { id: 8, marker: PhantomData<fn() -> core::any::TypeId> }, type_name: Some("[u64; 4]"), docs: [] }] }), docs: [] }

Could you take a look?

@davidsemakula
Copy link
Contributor Author

@cmichi should be fixed - hadn't considered these types at all 🙂

internal/lang-err/call-buildershould still choke on an Option<T> return type, but I guess that's expected behavior.

@davidsemakula davidsemakula force-pushed the solidity-compatible-metadata branch from a273fbf to 1723caa Compare February 5, 2025 21:08
Comment on lines 276 to 279
let ty = match path_segments.as_slice() {
// TODO: (@davidsemakula) should `primitive_types::H160` be bytes20?
["ink_primitives", "types", "AccountId"]
| ["primitive_types", "H160"] => "address",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should primitive_types::H160 be bytes20?
Also should ink_primitives::types::AccountId be bytes32?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean if primitive_types::H160 should be mapped to bytes20 for the Solidity metadata? Is there no native "H160" type in Solidity, for contract addresses?

Copy link
Contributor Author

@davidsemakula davidsemakula Feb 6, 2025

Choose a reason for hiding this comment

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

You mean if primitive_types::H160 should be mapped to bytes20 for the Solidity metadata

Yes

Is there no native "H160" type in Solidity, for contract addresses?

That would be address (the current implementation), the question is more about the ink! side. Is this a "safe" mapping? Does primitive_types::H160 (in the signature of an ink! constructor/message/event) always represent an address or is it just a 20 bytes hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the Solidity side it's trivial to convert between address <-> bytes20 <-> uint160. I'm just wondering if its "sound" to assume primitive_types::H160 always represents an address on the ink! side.

Copy link
Collaborator

@cmichi cmichi Feb 6, 2025

Choose a reason for hiding this comment

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

That's a good point. There's nothing keeping people from just using the type as a general purpose [u8; 20] without any association to an address.

Hmm, maybe we should after all introduce an Environment::Address type instead of using H160 implicitly for that. That would give you more clarity on the semantic interpretation.

Copy link
Collaborator

@cmichi cmichi Feb 6, 2025

Choose a reason for hiding this comment

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

Do I understand you right that you mean in the sense of us providing a type ink_primitives::Byte = u8 for developers?

So if e.g. someone wants to ensure the following return value gets mapped to bytes2 in the Solidity metadata:

#[ink(message)]
fn get(&self) -> [Byte; 2]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly

Copy link
Contributor

Choose a reason for hiding this comment

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

For the existing AccountId = [u8;32], would that stay the same? And then would the mapping to Solidity be something like: AccountId == uint8[32].

Or would it become: AccountId = [Byte; 32] == bytes32?

One other thought: if an RLP encoded ink! message takes an AccountId(32) as a parameter, should we give any linting warnings or simply represent it as bytes32 (or similar)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or would it become: AccountId = [Byte; 32] == bytes32?

Yes ink_primitives::types::AccountId should be bytes32. There's no need to convert it to AccountId = [Byte; 32] though, the top level wrapper type already makes it unambigious

Copy link
Contributor Author

@davidsemakula davidsemakula Feb 6, 2025

Choose a reason for hiding this comment

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

if an RLP encoded ink! message takes an AccountId(32) as a parameter, should we give any linting warnings or simply represent it as bytes32 (or similar)?

I think we just represent it as bytes32 with no warnings since Solidity has no equivalent elementary/primitive type and the user/dev can't do anything about it.

Edit: Also I believe it's common for Solidity contracts to represent foreign (cross-chain) addresses as bytes<N> especially in cross-chain messaging protocols.

crates/build/src/solidity_metadata/abi.rs Outdated Show resolved Hide resolved
crates/build/src/solidity_metadata/abi.rs Outdated Show resolved Hide resolved
crates/build/src/lib.rs Outdated Show resolved Hide resolved
util::base_name(&metadata_result.dest_metadata).bold(),
match metadata_result.spec {
MetadataSpec::Ink => "metadata",
MetadataSpec::Solidity => "Solidity compatible ABI",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
MetadataSpec::Solidity => "Solidity compatible ABI",
MetadataSpec::Solidity => "Solidity compatible metadata",

For a consistent naming throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this one, this line is for the <artifact_name>.abi file. See image below:
Screenshot 2025-02-07 at 15 43 27

With this suggested change, this becomes
Screenshot 2025-02-07 at 15 52 36

errors: HashMap<String, ItemDevDoc>,
}

/// NatSpec user documentation of the contract.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain in a comment somewhere how developers can influence which text lands in NatSpec and which in DevDoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So for Solidity, NatSpec (https://docs.soliditylang.org/en/latest/natspec-format.html) has a concept of "tags" (e.g. @dev and @notice) that represent the target audience (e.g. developer or user) for specific lines in docs e.g.

/// @notice An NFT
/// @dev Inherits OpenZepplin ERC721 implementation

ink!/Rust don't really have a "native" equivalent to this, and I'm not even sure we need one because explorers seem to show both AFAICT. So I think we just treat ink!/Rust comments as DevDoc for now, and ignore (leave empty) the UserDoc fields.

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.

[DEV] cargo-contract: emit eth ABI file Eth ABI compatibility: emit eth ABI file
3 participants