-
Notifications
You must be signed in to change notification settings - Fork 29
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 dynamic array encoding for encodeData #394
feat: add dynamic array encoding for encodeData #394
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #394 +/- ##
===========================================
- Coverage 83.71% 82.05% -1.66%
===========================================
Files 18 19 +1
Lines 1130 1332 +202
Branches 255 313 +58
===========================================
+ Hits 946 1093 +147
- Misses 98 130 +32
- Partials 86 109 +23 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update the function JSDoc at lines 231, 232. It is missing the 2 new inputs.
Can you update the docs and add examples on this new feature?
Nice tests
Here this page needs to be updated: https://docs.lukso.tech/tools/erc725js/classes/ERC725#encodedata Source: https://github.com/lukso-network/docs/blob/main/docs/tools/erc725js/classes/ERC725.md#encodedata Btw, I see we have 2 infos inside of each other, could you also fix that in this PR? |
Thanks for the review and feedback 🙏🏻 I renamed I think it's important to eliminate all questions, as this will be the second "big update" for dynamic user inputs, besides the already existing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
6d8eb41
to
7270593
Compare
Alright, I figured out how to adjust the error messages. Had to adjust the tests to also include these brackets, as they were looking for a close match. |
What kind of change does this PR introduce?
Feature: Dynamically encode ERC725 storage keys of type Array.
What is the current behavior?
The library only allows users to define the full array they want to encode and set to a contract. For updates, developers must read through all existing elements and pay the gas for re-setting elements in the list, even if only a few of them are actually updated. This is highly impractical.
What is the new behavior?
Within the encodeData(...) function, developers can define a startingIndex and an arrayLenght parameter to dynamically adjust the output, which can be used for more gradient storage updates. Both parameters are optional. If not provided, the encoding falls back to the default behavior as before, starting from index 0 and using the length of the provided elements in the array.