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 support for bool (valueType) and Boolean (valueContent) #266

Merged
merged 6 commits into from
Feb 6, 2023

Conversation

CJ42
Copy link
Collaborator

@CJ42 CJ42 commented Jan 31, 2023

What kind of change does this PR introduce (bug fix, feature, docs update, ...)?

⭐ Feature

What is the current behaviour (you can also link to an open issue here)?

It is not possible to use erc725.js library to:

  • encode/decode bool or an array of bool[] as value types.
  • decode data keys that contain Boolean in the ERC725Y JSON Schema provided.

What is the new behaviour (if this is a feature change)?

  • add support for encoding/decoding bool and bool[] as value type.
  • add support for decoding data to be interpreted as Boolean

Other information:

Supersedes + closes #212
closes issue #202

@CJ42 CJ42 requested review from Hugoo and CallumGrindle January 31, 2023 12:32
test/mockSchema.ts Outdated Show resolved Hide resolved
@Hugoo
Copy link
Contributor

Hugoo commented Feb 1, 2023

@CJ42 the standard mentions boolean, not bool:
https://github.com/lukso-network/LIPs/blob/main/LSPs/LSP-2-ERC725YJSONSchema.md#valuetype

image

Shall we:

  1. Update the standard to add bool AND add boolean support in this PR?
  2. Update the standard to replace boolean to bool?
  3. Change this PR to support boolean instead of bool ?

I think 3. is the way to go?

@CJ42
Copy link
Collaborator Author

CJ42 commented Feb 1, 2023

@Hugoo that's a very good point to have spotted! 👌
I can change it to boolean for now.

However, we should either:

  • go with option 2) in the future
  • allow both bool and boolean

So in the LSP2 standard, change boolean by bool. Simply because the valueType is for encoding/decoding in the smart contract, according to the smart language (Solidity in our case).

image

In Solidity </> --> the type is bool ✅ (not boolean ❌ ).

But in plain English 🗣️ -> the term boolean is quite a common terminology. (It's also boolean that is commonly used in other programming languages too like Javascript). We say in plain english "a boolean value" ✅, not "a bool value"

So maybe we should allow both in the Standard?

Let's have @frozeman input on this.

@CJ42
Copy link
Collaborator Author

CJ42 commented Feb 2, 2023

@Hugoo we changed boolean by bool in LSP2: https://github.com/lukso-network/LIPs/pulls
In the future we should allow both to write either boolean or bool. boolean would fallback to encoding/decoding as bool. Opened a feature request: #268

@CJ42
Copy link
Collaborator Author

CJ42 commented Feb 2, 2023

Implemented boolean as well. Closes #268

Copy link
Contributor

@Hugoo Hugoo left a comment

Choose a reason for hiding this comment

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

i think there are still a few missing things

src/lib/encoder.ts Show resolved Hide resolved
src/types/ERC725JSONSchema.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2023

Codecov Report

Merging #266 (c4019e6) into develop (122efa9) will increase coverage by 0.77%.
The diff coverage is 90.09%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop     #266      +/-   ##
===========================================
+ Coverage    83.04%   83.81%   +0.77%     
===========================================
  Files           18       18              
  Lines          979     1075      +96     
  Branches       218      232      +14     
===========================================
+ Hits           813      901      +88     
- Misses          91       96       +5     
- Partials        75       78       +3     
Impacted Files Coverage Δ
src/lib/utils.ts 83.01% <72.72%> (-0.88%) ⬇️
src/lib/decodeData.ts 82.40% <86.20%> (+1.50%) ⬆️
src/lib/encoder.ts 82.77% <94.28%> (+4.86%) ⬆️
src/constants/constants.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Hugoo Hugoo merged commit 86d606e into develop Feb 6, 2023
@Hugoo Hugoo deleted the feat/boolean branch February 6, 2023 15:52
This was referenced May 15, 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.

5 participants