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

Simplify/refactor fxparam (de)serializers/handlers #18

Closed
wants to merge 7 commits into from

Conversation

postspectacular
Copy link

I've been working on simplifying & updating the various fxparam (de)serializers, originally driven by the desire to keep the new boilerplate as lightweight as possible (there's a related PR forthcoming in that repo too). However, then I also spotted a few other things here, which I think this PR improves on:

Numbers / bigints

Simplified both types via better re-use of memory mapped typed arrays and other new, tiny helper functions...

Color

  • update/simplify rgbaToHex()
  • update completeHexColor():
    • add 4-digit hex color support
    • fix: enforce 8-digit result (even if incomplete input)

Strings

Strings are now encoded (and decoded) via the native TextEncoder API. This uses UTF-8 now and ensures all multi-byte unicode chars are properly and completely encoded (using String.charCodeAt() on its own, without further logic does not guarantee that). Please also see further comments in the source (and commit messages)...

Select

Ensure serialized value is clamped to 0-255 (even if given invalid input). Update use of default value

Summary

  • add U8/32/64 hex formatters
  • add asBytes() hexbyte string parser
  • add minmax() helper for enforcing optional ranges and dedupe logic
  • update/simplify serializers/deserializers for all param types
  • fix string param serialization to safely handle non-ASCII chars (using UTF-8 representation)
  • add/update docstrings

As mentioned, I've also applied the same changes to the Snippet API in the fxhash-boilerplate repo. Will open a related PR for that in a bit (have to still merge the recent changes re: string maxLength)...

Hth! 👍

Major simplifications of various fxparam related functions:
- add U8/32/64 hex formatters
- add asBytes() hexbyte string parser
- add minmax() helper for enforcing optional ranges and dedupe logic
- update/simplify rgbaToHex()()
- update completeHexColor():
  - add 4-digit hex color support
  - fix: enforce 8-digit result (even if incomplete input)
- update/simplify serializers/deserializers for all param types
- fix string param serialization to safely handle non-ASCII chars
  (using UTF-8 representation)
- add/update docstrings
* upstream/master:
  build
  build
  fix: only push params if definitions defined in code
  build
  added minter address to the panel
  update build
  receive snippet version; allow user defined max length on string param
  only warn type any
@postspectacular
Copy link
Author

There's one other thing I'd like to get more clarity on (and which might need some additional small adjustments):

Default handling.

In the FxParamDefinition interface default values are not considered optional, but in some parts of the actual de/serializers they are treated as such (but not consistently). It'd be good to get clarity about how this should be handled in general and/or how to deal with missing defaults. Once I hear back from you I can update the logic & this PR... Thanks! :)

@postspectacular
Copy link
Author

I'm not sure if this is the right place, since this is more to do with the boilerplate, but it's also very related to this PR and my above comment - so forgive me for adding even more here, but I think the current handling of missing default values for declared fxparams should be re-considered, since it leads to non-deterministic behavior if the fxparam URL query string is not given to the project.

Whereas using Math.random() for interactively randomizing parameters works just fine for the fxlens UI in this repo here, using the same to deal with missing defaults in the boilerplate will lead to unexpected variations, at least during development (without using fxlens), but there might also be other situations. I think it would make more sense for the boilerplate version of those parameter randomize() methods to use fxrand() instead of Math.random() to ensure deterministic outcomes... What are your thoughts?

If the mere presence of params declared via $fx.params(...) will force minters to use define/randomize params and then ensures the fxparam query string is always provided to the project, maybe this is less of an issue, but I'd still think that switching the PRNG is conceptually/philosophically more sound, safe (as in avoiding unexpected outcomes) and more aligned with other parts of fxhash.

@dirx
Copy link
Contributor

dirx commented May 28, 2023

@maerzhase
Copy link
Contributor

its alright we have seen it and will integrate the improvements soon ™️
we were kept busy on multiple grounds, soon there will be a lot of improvements in code quality <3

@maerzhase
Copy link
Contributor

couldn't push on your branch. changes are incorporated in #22

@maerzhase maerzhase closed this Jun 16, 2023
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