-
Notifications
You must be signed in to change notification settings - Fork 432
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
Fix build warnings + clippy errors from latest nightly #686
Fix build warnings + clippy errors from latest nightly #686
Conversation
This PR is blocked until ferrilab/funty#3 is fixed. |
Might be worth of putting together a temporary workaround instead. |
.replace("]", "") | ||
.split(", ") | ||
.map(|s| u8::from_str(s).expect("u8 cannot be extracted from str")) | ||
.collect::<Vec<u8>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike this hacky way of deserializing, but AFAIK it's the simplest way of getting the inverse of a panic!("{:?}", Vec<u8>)
. We can get rid of this as soon as panic_any
is stablized, which should be with the next stable release soonish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create an issue for that so that we do not forget?
please ignore |
@@ -277,7 +277,7 @@ impl EnvInstance { | |||
beneficiary, | |||
transferred: all, | |||
}; | |||
panic!(scale::Encode::encode(&res)); | |||
panic!("{:?}", scale::Encode::encode(&res)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this actually should be converted into an unimplemented!
with a proper message stating why it is still unimeplemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so I'm not sure.
I mean, even if we implement setting a tombstone, clearing storage, etc. we would still like to return the result of the contract eviction and it seems that panic!
is the right way to go.
From my pov your suggestion would just be a semantic change of having something like unimplemented!("we have not yet implemented everything, but anyway here's the result: …")
instead and changing that back to panic!(…)
once we have implemented it would then be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well hopefully this discussion is not needed soon due to the works on the upcoming off-chain environment. :P But since contract termination is not really properly implemented in the current off-chain environment I would not dare to actually just mark it as unimplemented. But yeah let's keep it as it is for now.
.replace("]", "") | ||
.split(", ") | ||
.map(|s| u8::from_str(s).expect("u8 cannot be extracted from str")) | ||
.collect::<Vec<u8>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create an issue for that so that we do not forget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
See https://gitlab.parity.io/parity/ink/-/pipelines/123903 for the warnings we get now.