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: add uint2str builtin #2879

Merged
merged 12 commits into from
Jun 8, 2022
Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented May 27, 2022

What I did

implement #2703

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

image

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2022

Codecov Report

Merging #2879 (258b069) into master (02d02db) will increase coverage by 0.01%.
The diff coverage is 90.32%.

@@            Coverage Diff             @@
##           master    #2879      +/-   ##
==========================================
+ Coverage   87.94%   87.96%   +0.01%     
==========================================
  Files          94       95       +1     
  Lines       10257    10389     +132     
  Branches     2478     2502      +24     
==========================================
+ Hits         9021     9139     +118     
- Misses        783      794      +11     
- Partials      453      456       +3     
Impacted Files Coverage Δ
vyper/builtin_functions/functions.py 91.68% <90.32%> (-0.05%) ⬇️
vyper/codegen/stmt.py 88.16% <0.00%> (ø)
vyper/ast/pre_parser.py 96.82% <0.00%> (ø)
vyper/codegen/module.py 94.39% <0.00%> (ø)
vyper/semantics/types/bases.py 84.34% <0.00%> (ø)
vyper/ast/signatures/interface.py 81.48% <0.00%> (ø)
vyper/semantics/types/__init__.py 100.00% <0.00%> (ø)
vyper/semantics/types/user/enum.py 80.70% <0.00%> (ø)
vyper/ast/nodes.py 94.08% <0.00%> (+0.02%) ⬆️
vyper/exceptions.py 94.59% <0.00%> (+0.04%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02d02db...258b069. Read the comment docs.

@@ -1807,6 +1807,64 @@ class Max(_MinMax):
_opcode = "gt"


class UintToStr(_SimpleBuiltinFunction):
_id = "uint2str"
_inputs = [("x", Uint256Definition())] # should allow any uint?
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to allow any integer value, yes (including signed)

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't know. in the interest of simplicity, i think we should leave it as is for now, and then maybe add more types if people complain.

Copy link
Contributor

@skellet0r skellet0r May 30, 2022

Choose a reason for hiding this comment

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

Yeah, plus can always do some combination of converting and using abs built in.

For signed:

return concat("-", str(abs(some_signed_int)))

class UintToStr(_SimpleBuiltinFunction):
_id = "uint2str"
_inputs = [("x", Uint256Definition())] # should allow any uint?
_return_type = StringDefinition(78)
Copy link
Member

Choose a reason for hiding this comment

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

Integer type would just affect this number somehow

_id = "uint2str"
_inputs = [("x", Uint256Definition())] # should allow any uint?
_return_type = StringDefinition(78)

Copy link
Member

Choose a reason for hiding this comment

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

Also would be interesting to have kwargs to limit the total value if known beforehand (e.g. 10k pfp NFTs)

@@ -1807,6 +1807,64 @@ class Max(_MinMax):
_opcode = "gt"


class UintToStr(_SimpleBuiltinFunction):
_id = "uint2str"
Copy link
Member

Choose a reason for hiding this comment

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

maybe num2str is more general for other integer types

Copy link
Member Author

Choose a reason for hiding this comment

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

how about str() after python?

@fubuloubu
Copy link
Member

Can also add implementation to here: https://github.com/vyperlang/vyper/blob/master/examples/tokens/ERC721.vy

baseURL: String[53]  # ipfs://Qm....

@view
@external
def tokenURL(tokenId: uint256) -> String[132]:
    return concat(self.baseURL, uint2str(tokenId))

@skellet0r
Copy link
Contributor

I can already sense the usage of this with a constant ...
uint2str(3000) 🤦‍♀️

Would adding an evaluate method for compile time check be good here.

@charles-cooper
Copy link
Member Author

I can already sense the usage of this with a constant ... uint2str(3000) woman_facepalming

Would adding an evaluate method for compile time check be good here.

🤦 "3000" is even fewer characters than uint2str(3000). i mean maybe we should even block uint2str on literals

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
@charles-cooper
Copy link
Member Author

facepalm "3000" is even fewer characters than uint2str(3000). i mean maybe we should even block uint2str on literals

never mind, it makes sense -- 3000 could be a result of constant propagation

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.

My one misgivings with this PR is that I don't think it would be possible to signal when a type doesn't need the full string size for itself, like when converting tokenId for a 10k token mint set

@charles-cooper
Copy link
Member Author

i changed the builtin to allow a uint of any width, this also gives fine-grained control over the output string length.

@charles-cooper
Copy link
Member Author

switching back to uint2str, since we don't want to suggest that we have general str implementations for everything. if we start to add more builtins, we can try to settle on a more "generic" name like str or format.

@charles-cooper charles-cooper enabled auto-merge (squash) June 8, 2022 03:24
@charles-cooper charles-cooper merged commit ac8cb6e into vyperlang:master Jun 8, 2022
@charles-cooper charles-cooper deleted the uint2str branch June 8, 2022 04:53
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

4 participants