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

feat[lang]!: change ABI type of decimal to int168 #3696

Merged
merged 49 commits into from
Apr 10, 2024

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Dec 16, 2023

going forward, vyper should expose decimal types as integers in the ABI.

What I did

this commit changes how decimals are exposed in the ABI - changing
the ABI type from `fixed168x10` to `int168`. this is done for
compatibility reaosns, since `fixedMxN` types are not widely supported.
it is also a preparatory breaking change for supporting more decimal
types, in the future, we might expand the set of decimal types, e.g.
something along the lines of `Decimal[bits, places]`. in such a future,
if we add `UDecimal[256, 18]` (colloquially known as `"wad"`), it would
be useful to users if it had an ABI type of uint256, for compatibility
with common ERCs.

to distinguish from plain `int168` in the JSON ABI, an `"internalType"`
field is added, which includes the decimal info. in the future, if we
add more decimal types, it would be distinguished from other decimal
types according to the metadata, ex.:
`{"internalType": {"decimal": {"bits": 168, "places": 10}}}`.

misc/refactor:
- remove `FixedMxN` abi types
- add a `decimal_to_int()` utility function to make test migration
  cleaner. it essentially emulates what the compiler does internally during
  codegen, which is that it takes the arguments to a `Decimal` and then
  bitcasts the resulting `Decimal` to an int.

How I did it

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

going forward, vyper should expose `decimal` types as integers in the
ABI.
@charles-cooper
Copy link
Member Author

note there will probably be some tests that need to be rewritten to use wad-type things instead of floats/decimals

Copy link
Collaborator

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

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

Please adjust the docs as well: https://github.com/vyperlang/vyper/blob/master/docs/types.rst?plain=1#L240. Also, I'm wondering why no tests have to be adjusted here, i.e. there is a missing test coverage here, that ensures the correct ABI type for decimal. We should add such a test as well IMO.

@charles-cooper
Copy link
Member Author

Please adjust the docs as well: https://github.com/vyperlang/vyper/blob/master/docs/types.rst?plain=1#L240. Also, I'm wondering why no tests have to be adjusted here, i.e. there is a missing test coverage here, that ensures the correct ABI type for decimal. We should add such a test as well IMO.

i don't think there are missing tests -- you can see there are failing tests which i haven't updated yet

Screenshot from 2023-12-17 11-30-43

@pcaversaccio
Copy link
Collaborator

Please adjust the docs as well: https://github.com/vyperlang/vyper/blob/master/docs/types.rst?plain=1#L240. Also, I'm wondering why no tests have to be adjusted here, i.e. there is a missing test coverage here, that ensures the correct ABI type for decimal. We should add such a test as well IMO.

i don't think there are missing tests -- you can see there are failing tests which i haven't updated yet

Screenshot from 2023-12-17 11-30-43

Oh yeah, weird, my GH UI didn't display the CI checks previously...

@charles-cooper
Copy link
Member Author

also re. the docs -- i've been thinking maybe we should move them to a separate repo?

@pcaversaccio
Copy link
Collaborator

also re. the docs -- i've been thinking maybe we should move them to a separate repo?

I'm not fully convinced this is a good idea. Vyper docs are still lacking a lot of insights, and having them part of the main repo helps to bundle the code and documentation changes in on PR. I'm afraid, that in the main repo a lot of undocumented activity will happen since maintaining a separate docs repo creates overhead.

@charles-cooper
Copy link
Member Author

charles-cooper commented Dec 17, 2023

I'm not fully convinced this is a good idea. Vyper docs are still lacking a lot of insights, and having them part of the main repo helps to bundle the code and documentation changes in on PR. I'm afraid, that in the main repo a lot of undocumented activity will happen since maintaining a separate docs repo creates overhead.

i think it could actually decrease overhead, because docs changes would not need to go through the review and CI process (the latter of which can take 30-60 minutes depending on CI busy-ness)

@pcaversaccio
Copy link
Collaborator

i think it could actually decrease overhead, because docs changes would not need to go through the review and CI process (the latter of which can take 30-60 minutes depending on CI busy-ness)

I think this is a non-issue, and having a review of the docs means additional QA in the context of the code-changing PR. If it's just a small documentation fix (w/o code change), 60 mins of CI don't matter, really. I personally think the current as-is setup is more streamlined, particularly given the team size.

@charles-cooper charles-cooper mentioned this pull request Feb 10, 2024
33 tasks
@charles-cooper charles-cooper marked this pull request as draft March 5, 2024 02:09
@charles-cooper charles-cooper added this to the v0.4.0 milestone Apr 4, 2024
@charles-cooper charles-cooper changed the title feat: change ABI type of decimal to int168 feat[ux]!: change ABI type of decimal to int168 Apr 5, 2024
@charles-cooper charles-cooper marked this pull request as ready for review April 8, 2024 12:05
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Seems okay, but given future plans I would suggest deprecation instead to reduce confusion around using this type (note: re-introduction would be non-breaking if this were the case)

@charles-cooper charles-cooper enabled auto-merge (squash) April 9, 2024 23:54
@charles-cooper charles-cooper merged commit b43ffac into vyperlang:master Apr 10, 2024
148 checks passed
electriclilies pushed a commit to electriclilies/vyper that referenced this pull request Apr 27, 2024
this commit changes how decimals are exposed in the ABI - changing
the ABI type from `fixed168x10` to `int168`. this is done for
compatibility reaosns, since `fixedMxN` types are not widely supported.
it is also a preparatory breaking change for supporting more decimal
types, in the future, we might expand the set of decimal types, e.g.
something along the lines of `Decimal[bits, places]`. in such a future,
if we add `UDecimal[256, 18]` (colloquially known as `"wad"`), it would
be useful to users if it had an ABI type of uint256, for compatibility
with common ERCs.

to distinguish from plain `int168` in the JSON ABI, an `"internalType"`
field is added, which includes the decimal info. in the future, if we
add more decimal types, it would be distinguished from other decimal
types according to the metadata, ex.:
`{"internalType": {"decimal": {"bits": 168, "places": 10}}}`.

misc/refactor:
- remove `FixedMxN` abi types
- add a `decimal_to_int()` utility function to make test migration
  cleaner. it essentially emulates what the compiler does internally during
  codegen, which is that it takes the arguments to a `Decimal` and then
  bitcasts the resulting `Decimal` to an int.

---------

Co-authored-by: tserg <8017125+tserg@users.noreply.github.com>
esaulpaugh added a commit to esaulpaugh/headlong that referenced this pull request Apr 29, 2024
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.

None yet

6 participants