-
Notifications
You must be signed in to change notification settings - Fork 13
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
Update APIs to use MobileCoinRng instead of RngSeed #197
Update APIs to use MobileCoinRng instead of RngSeed #197
Conversation
would be great to have a test for this, @dolanbernard can you provide a test data from Android to cross-test? |
completion: @escaping ( | ||
Result<PendingSinglePayloadTransaction, TransactionPreparationError> | ||
) -> Void | ||
) { | ||
guard let rngSeed = rng.generateRngSeed() else { |
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.
not sure this is correct, if you only use the RNG to create a seed which you then pass around to see our internal RNG, that defeats the purpose of letting them pass in their own RNG class
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.
Android uses the input Rng to seed the RNG for output idempotence. So here, we are doing the same as Android with respect to the output transaction creation.
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 then, doesnt seem to be as flexible as I initially thought, but if its the same as android thats correct
completion: @escaping ( | ||
Result<PendingSinglePayloadTransaction, TransactionPreparationError> | ||
) -> Void | ||
) { | ||
guard let rngSeed = rng.generateRngSeed() else { |
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.
same feedback here
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.
same answer as above :)
Test has been added with matching data (passing successfully) |
Soundtrack of this PR: A 'random' InSync song lol
Motivation
Sync up iOS and Android's implementation of idempotence for cross-platform compatibility
In this PR