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

fix: float64 with 7 digits are printing out with scientific notation #90

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

mimy2001
Copy link

Two issues that this PR deals with

  1. We had a filing which was $1,252,363.25 and this produced scientific notation looking like 1.252364e+06 which fincen doesn't accept. See image attached and code looking like: <fc2:EFilingBatchXML TotalAmount="1.252364e+06" PartyCount="1"

image (8)

After this change it becomes:
<fc2:EFilingBatchXML TotalAmount="1252363" PartyCount="1" ActivityCount="1"

  1. For SAR filings with 0 amounts, the total amount field is omitted because it's omit empty, we used to do something hacky in our code which adds this element with 0 value but taking out the omit empty tag will fix this issue as well

Our workaround looked like this:
if totalAmount == 0 { fincenReport.Attrs = append( fincenReport.Attrs, xml.Attr{ Name: xml.Name{Local: "TotalAmount"}, Value: "0", }) }

@mimy2001 mimy2001 requested a review from adamdecaf as a code owner January 12, 2024 03:18
@mimy2001 mimy2001 changed the title fix: float64 with 8 digits are printing out with scientific notation fix: float64 with 7 digits are printing out with scientific notation Jan 12, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (b631573) 73.74% compared to head (e1c8787) 73.61%.

❗ Current head e1c8787 differs from pull request most recent head f5d4d03. Consider uploading reports for the commit f5d4d03 to get more accurate results

Files Patch % Lines
pkg/batch/batch.go 0.00% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   73.74%   73.61%   -0.13%     
==========================================
  Files          17       17              
  Lines        1699     1702       +3     
==========================================
  Hits         1253     1253              
- Misses        387      390       +3     
  Partials       59       59              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adamdecaf
Copy link
Member

Thanks for this fix! It looks like Go will use scientific notation because the XML spec supports it but obviously FinCEN doesn't. Are there other float64 fields to make this fix for too?

@adamdecaf adamdecaf merged commit 3fce8e8 into moov-io:master Jan 12, 2024
5 checks passed
adamdecaf added a commit to moov-io/rtp20022 that referenced this pull request Jan 12, 2024
@mimy2001
Copy link
Author

@adamdecaf I only noticed it with total amount, but technically this could happen for any other float64 fields. Do you want me to also see if I should make this fix elsewhere as well? And that's right, go uses scientific notation due to XML, if this was json we wouldn't have an issue. Even though XML spec supports this, fincen doesn't, we tried this and it failed with errors.

@adamdecaf
Copy link
Member

adamdecaf commented Jan 12, 2024

Yea, this change is fine. I see maybe one other spot to fix this.

@mfdeveloper508 Do you know why we copied in some code from Go's repository? Why do we have a copy of their xml marshaling?

fincen/marshal.go

Lines 915 to 916 in 3fce8e8

case reflect.Float32, reflect.Float64:
return strconv.FormatFloat(val.Float(), 'g', -1, val.Type().Bits()), nil, nil

Edit: If I remove our copy of MarshalIndent no tests fail, so why was it needed?

@mimy2001
Copy link
Author

@adamdecaf I just realized something, the fix I need is not completely correct, that number needs to round up :( I can submit another PR today when i get a chance or if you have time maybe you can. But I simply made is so it's no precision to print out the integer part of the float but that's not correct, the amount always rounds up to the next whole number no matter what from what my PM just told me. Do you want me to take this on or is that something you want to do quickly?

@adamdecaf
Copy link
Member

adamdecaf commented Jan 12, 2024

Is the behavior to round up unique to this field? Adding a roundUpFloat type sounds like the solution to me.

@mimy2001
Copy link
Author

Just thinking this one through, I've been trying to find the CTR/SAR guide where it gives guidance on round up/round down or just truncate and I can't find definitive verbiage on that. So this could also just be dealt with client side thought by prerounding before using this field. I might be able to work around this without a change to the library. And for the record when it was using scientific notation here, it did print out rounding it up to the next dollar. Let me know if you think i should submit any code for this.

@adamdecaf
Copy link
Member

If this field always needs to be rounded up we can implement that in the library. Renaming the type added in this PR to add round-up behavior works for me.

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.

3 participants