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

types: Make (Currency).String exact #146

Merged
merged 2 commits into from Mar 15, 2024
Merged

types: Make (Currency).String exact #146

merged 2 commits into from Mar 15, 2024

Conversation

lukechampine
Copy link
Member

Old: "~2.529 mS"
New: "2.529378333356156158367 mS"

We discussed removing the SI units as well, and using SC exclusively (i.e. the example above would be 2.529378333356156158367 mS), but there was less consensus on that.

Since String is now exact, ExactString has been removed; if you want raw Hastings values, you can use Big or fmt.Sprintf("%d", c). If you want a rounded value, you can use standard approaches to round the float64 returned by Siacoins.

@n8maninger
Copy link
Member

n8maninger commented Mar 11, 2024

I'm okay with the change to String(), but I'm against the removal of ExactString().

Personally, I'd rather have String() return the value in hastings. All code works with hastings and a Currency represents a number of hastings, not Siacoin.

@lukechampine
Copy link
Member Author

fair enough, restored.

I've also tweaked UnmarshalText to accept strings with units. This not only benefits weirdos like me who write JSON transactions by hand, but also anyone using Currency values in a human-readable config format (e.g. a YAML file specifying host prices? idk)

Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got a bit nervous so I pushed a small commit to cover UnmarshalText.

@n8maninger n8maninger merged commit c9bd79d into master Mar 15, 2024
8 checks passed
@n8maninger n8maninger deleted the currency-string branch March 15, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants