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

Refactoring bindings #234

Merged
merged 24 commits into from
Jul 8, 2019
Merged

Refactoring bindings #234

merged 24 commits into from
Jul 8, 2019

Conversation

maciejhirsz
Copy link
Contributor

@maciejhirsz maciejhirsz commented Apr 2, 2019

This PR abstracts out bindings in a way that makes iOS and Android extern functions DRY. Type conversion and error handling (throwing exceptions in Java) is done automagically via the export! macro. This also removes a whole bunch of unwrap and expects smell.

It also adds native function for generating QR codes in Rust, fixes #219.

  • Introduce error handling for iOS.
  • Update C header file.
  • Rework string deallocating in Swift bridge.
  • Test that iOS compiles and works (it won't until all of the above are done).
  • Make Android use KeyPair pointers similar to how iOS was done before.

@pmespresso
Copy link
Contributor

@maciejhirsz is this ready for testing on iOS?

@maciejhirsz
Copy link
Contributor Author

@yjkimjunior yes please! :)

@pmespresso
Copy link
Contributor

@maciejhirsz the build is failing for me because signer uses an older version of Swift than Xcode10.2 supports. Should this be updated, or should I just go downgrade my Xcode?

@maciejhirsz
Copy link
Contributor Author

If we can upgrade it I think we should. I think most (all?) of our swift code is internally using just objective C, so there shouldn't be any breakage, but in case there is feel free to ping me on Riot.

@pmespresso
Copy link
Contributor

compiles and no more memory allocation errors, able to create and restore accounts, however, I am able to delete it after i type in just 4 out of 6 of my pin digits. http://g.recordit.co/C7maeWWAWi.gif

@Tbaut
Copy link
Contributor

Tbaut commented Jun 26, 2019

Also I see on your recording that the qr code doesn't load and remains white, right?

@pmespresso
Copy link
Contributor

Ah yes, that too.

@maciejhirsz maciejhirsz marked this pull request as ready for review July 2, 2019 11:35
@maciejhirsz
Copy link
Contributor Author

So everything compiles at last, and the QR code is showing. Need to figure out what's up with account deletion though.

@pmespresso
Copy link
Contributor

leaving this here reference http://g.recordit.co/co1sYl9LrF.gif

@Tbaut
Copy link
Contributor

Tbaut commented Jul 2, 2019

I've touched this logic last here: https://github.com/paritytech/parity-signer/pull/195/files#diff-ed7c745a0e11d1facbcbeca7e4a54c0aL46 and it might not have been tested enough on iphone

@maciejhirsz
Copy link
Contributor Author

JS looks fine, just a bit convoluted.

It might be possible that errors aren't propagated properly for iOS on this branch, which is what we use to check if the pin is correct. I'll dig deeper tomorrow to see if I can either figure this out or at least mock something to confirm or reject the hypothesis I have now.

@pmespresso
Copy link
Contributor

@maciejhirsz works well for me!

@maciejhirsz maciejhirsz merged commit eb55ff6 into master Jul 8, 2019
@maciejhirsz maciejhirsz deleted the mh-clean-up-bindings branch July 8, 2019 09:03
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.

Blank QR code on accounts page
3 participants