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

Add encode command #998

Merged
merged 4 commits into from Mar 2, 2023
Merged

Add encode command #998

merged 4 commits into from Mar 2, 2023

Conversation

SkymanOne
Copy link
Contributor

Closes #986

Some changes:

  • extracted contract_artifacts() in extrinsics into a separate public function find_contract_artifacts() because we need it in order to retrieve the metadata, but ExtrinsicOpts also require --suri to be specified that we don't need for encoding.
  • Added additional E2E test to test encoding

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of nitpicks and questions

crates/cargo-contract/src/cmd/extrinsics/mod.rs Outdated Show resolved Hide resolved
crates/cargo-contract/tests/decode.rs Outdated Show resolved Hide resolved
let transcoder = artifacts.contract_transcoder()?;

let call_data = transcoder.encode(&self.message, &self.args)?;
let call_data_encoded = hex::encode_upper(call_data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have you chosen to use upper case hex? I don't mind it, but we should be mindful of consistency where we have used hex::encode in other places. If uppercase hex is preferable we should have it everywhere, I have never considered the pros and cons before though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that's how we specify custom selectors in tests in examples

Copy link
Collaborator

@ascjones ascjones Mar 2, 2023

Choose a reason for hiding this comment

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

True, but this is showing the whole encoded message not just the selector.

It seems to be a matter of taste whether hex values are preferred to be expressed as upper or lowercase. I don't really have an opinion, I just think it should be consistent within this tool. So if we prefer upper case hex representation of bytes then we should do that everywhere (in cargo-contract). Because previously the default was plain hex::encode which does lowercase.

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 did it in other places too

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can still find a few other places with hex::encode though (okay we don't want to change it for the LowerHex impls. It doesn't matter that much in the end though. Just nice to be consistent.

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

@SkymanOne SkymanOne merged commit c84f864 into master Mar 2, 2023
@SkymanOne SkymanOne deleted the gn/add-encode-command branch March 2, 2023 18:03
This was referenced Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sub-command cargo contract encode
2 participants