-
Notifications
You must be signed in to change notification settings - Fork 871
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
fixed WALLET_PROPERTIES_ST serialization #2228
Conversation
writer->String(data.probi_.c_str()); | ||
|
||
writer->String("balance"); | ||
writer->Double(data.balance_); | ||
writer->String(std::to_string(data.balance_).c_str()); |
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 agree using a String is safer, however right now the solution to be consistent would be to change d.HasMember("balance") && d["balance"].IsString()
to d.HasMember("balance") && d["balance"].IsDouble()
@NejcZdovc Would be great if you could check the above
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 think we discussed on a call that we are going to change all double to string internally. I don't think we should change it there.
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.
structure currently have it as string so we should leave it as string for now
LGMT to me other than comment to @NejcZdovc, also @SergeyZhukovsky can you tick the appropriate boxes in the description please, thanks |
I marked the checkboxes and commented on the above question to @NejcZdovc |
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.
Approving to unblock Android team. @NejcZdovc if there is anything you notice can you please create a follow up issue
closes brave/brave-browser#4115
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist: