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

Non-Standardized Supporting #236

Merged
merged 29 commits into from
Sep 13, 2021

Conversation

AurevoirXavier
Copy link
Contributor

@AurevoirXavier AurevoirXavier commented Sep 8, 2021

I revamp my old implementation #211.

  • All JSON tests were ignored under no-std.
  • Reader/Tokenizer/serde/serde_jsonshould be enabled by --features full-serde.
    • I'm not sure if I should disable Writer in no-std, because I found it was used in signature.
  • Error::FromUtf8Error removed, cause can't find any usage of it.
  • All HashMap change to BTreeMap for no-std. !!BREAKING change!!

@sorpaas

@AurevoirXavier AurevoirXavier changed the title Non Standardized Non-Standardized Supporting Sep 8, 2021
ethabi/src/contract.rs Outdated Show resolved Hide resolved
@sorpaas
Copy link
Member

sorpaas commented Sep 8, 2021

I'd appreciate if you can add CI build run for no_std as well!

@vkgnosis
Copy link
Member

vkgnosis commented Sep 8, 2021

Could you explain a bit on the motivation for no std support? What part of the crate is still useful without std? Why does HashMap have to change to BTreeMap?

@AurevoirXavier
Copy link
Contributor Author

AurevoirXavier commented Sep 9, 2021

Could you explain a bit on the motivation for no std support? What part of the crate is still useful without std? Why does HashMap have to change to BTreeMap?

  1. I found many people use this in substrate runtime, so do I. Some of them used it in EVM. And my team used it for the ethereum cross-chain bridge.
  2. Reader/Tokenizer/serde/serde_json not available in no-std.
  3. There is no HashMap under alloc. And we use BTreeMap to replace in general.

@AurevoirXavier
Copy link
Contributor Author

AurevoirXavier commented Sep 9, 2021

I'd appreciate if you can add CI build run for no_std as well!

Okay.

Interesting, why the CI failed?

cargo test --no-default-features passed locally.

@sorpaas
Copy link
Member

sorpaas commented Sep 9, 2021

Please remove check and test command in CI, and add build.

@AurevoirXavier
Copy link
Contributor Author

Please remove check and test command in CI, and add build.

Please review.

@sorpaas
Copy link
Member

sorpaas commented Sep 13, 2021

CI is failing!

@AurevoirXavier
Copy link
Contributor Author

CI is failing!

I see. cargo-hack is pretty strict.

@@ -28,7 +28,6 @@ paste = "1"

[features]
default = [
"std",
Copy link
Member

Choose a reason for hiding this comment

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

This is just wrong. By default std should be enabled.

Copy link
Contributor Author

@AurevoirXavier AurevoirXavier Sep 13, 2021

Choose a reason for hiding this comment

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

This is just wrong. By default std should be enabled.

But the full-serde contains std. Should I explicitly write it here again?

Copy link
Contributor Author

@AurevoirXavier AurevoirXavier Sep 13, 2021

Choose a reason for hiding this comment

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

Okay. I found a corner case. You're right.

Copy link
Contributor Author

@AurevoirXavier AurevoirXavier Sep 13, 2021

Choose a reason for hiding this comment

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

Sorry about ^^^

The full-serde is enabled by default. So std is always enabled by defualt.

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