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

Refactored Address to not be a partial class of Protobuf Address class #23

Merged
merged 1 commit into from
Jun 23, 2018

Conversation

ZimM-LostPolygon
Copy link
Contributor

@ZimM-LostPolygon ZimM-LostPolygon commented Jun 23, 2018

Address was the only Protobuf class from the SDK that was supposed to be used by end-users. Generally, this should not happen - Protobuf stuff is too low-level, and there's a reason why Protobuf classes are sealed - they are not supposed to be extended in any way. It was also quite annoying to see Protobuf-related methods in autocomplete when working with addresses.

This PR basically adds a nicer Address struct and hides all Protobuf stuff away from end users' eyes.

Noteworthy:

  • Public interface remained unchanged.
  • This change would allow to use Address directly when encoding/decoding EVM data with Nethereum, instead of using plain strings. It is currently quite annoying to juggle between Address and pure string representations of the same thing. This will make the usage patterns more uniform.
  • Local address hex string is now checksum-encoded.
  • Address instances can now be compared using == and != operators for convenience.
  • New implementation is slightly faster due to being a struct.
  • Debug.Log(address); now actually logs the address string
  • Since no Protobuf classes are now used publicly, they were moved in a separate namespace as to not clutter the autocompletion for end users.

@enlight
Copy link
Collaborator

enlight commented Jun 23, 2018

Very nice! Did you test AddressJsonConverter in WebGL?

@ZimM-LostPolygon
Copy link
Contributor Author

ZimM-LostPolygon commented Jun 23, 2018 via email

@enlight
Copy link
Collaborator

enlight commented Jun 23, 2018

Just the usual WebGL issues that crop up when symbols are stripped during the build. This looks safe enough though I suppose.

@enlight enlight merged commit d4a9952 into master Jun 23, 2018
@enlight enlight deleted the address-refactor branch June 23, 2018 19:18
@ZimM-LostPolygon
Copy link
Contributor Author

@enlight Ah. It's not just WebGL then, also iOS, and, optionally, Android and Desktop. But yes, this should be fine.

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.

2 participants