Skip to content
This repository has been archived by the owner on May 2, 2022. It is now read-only.

Conversation

awolokita
Copy link

This PR addresses the issue raised in #52 by implementing the change proposed by @achimcc.
Implementing this fix revealed the additional incorrect behaviour in handling Option types mentioned in #52.

The three major changes in this PR are

  1. Prefer type.type over type.displayName in formatData
  2. Use codec instead of value when checking for an Option type. Use codec.isSome and codec.unwrap.
  3. Remove the check for null and and empty array at the start of Data and instead let createTypeUnsafe handle any issues and throw an exception. Catch and return () in case of any exception.

Apologies in advance for the large number of lines changed. The bulk of these changes are due to an extra level of indentation as a result of the try-catch. Any feedback on how to do this more cleanly would be greatly appreciated.

awolokita added 6 commits May 31, 2021 22:05
The use of `value` for handling `Option` types meant that the code in the if statement was never executed and the hex representation of the type was returned.

This commit uses `codec` instead of `value` and `unwrap`s the data when it `isSome`.
The check for `null` at the start of the `Data` function meant that an `Option` that `isNone` would return `()`. This was because `value` for `Option.isNone == true` is `null`.

Instead, we can wrap all the type handlers in a try-catch, and let `createTypeUnsafe` in `formatData` deal with our invalid values (and throw if something bad happens).

Apologies for the large number of changed lines - the try-catch required an additional level of indentation.
Missed checking in the hunk that removes the check for null in `Data`.
Moves the try-catch into formatData and change it to return a `Null` `Codec` instance instead of `null`. Replace `isNull` with `isInstanceOf(code, Null)`.

Undo indentation.
@awolokita
Copy link
Author

Opted for doing try-catch in formatData and return Null Codec instance instead of Null for better overall integration.

@awolokita
Copy link
Author

awolokita commented Jun 1, 2021

Also fixes #13.

@awolokita awolokita closed this Jul 5, 2021
@awolokita awolokita reopened this Jul 5, 2021
@kwingram25 kwingram25 merged commit 43c545c into paritytech:master Jul 6, 2021
cmichi added a commit to paritytech/ink-waterfall that referenced this pull request Jul 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants