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 isVeWorldSupported variable #259

Merged
merged 5 commits into from
Mar 19, 2024
Merged

feat: add isVeWorldSupported variable #259

merged 5 commits into from
Mar 19, 2024

Conversation

davidecarpini
Copy link
Member

No description provided.

@darrenvechain
Copy link
Member

darrenvechain commented Mar 7, 2024

@davidecarpini instead of having a field isVeWorldCompatible could we introduce it as a tag instead? I know that introducing new fields could potentially break existing applications. Not Node JS, but Java/ Kotlin, probably others

Something like:

"tags": [
  "existing-tag",
  "veworld"
],

@davidecarpini
Copy link
Member Author

Hey @darrenvechain it's not a problem to move everything to a tag, but I think it's not a good idea since a tag is more a characteristic of the dapp, rather than a behavior.
tbh if the code explodes because we add an attribute in the json it's probably not great code 😄
said that, you are right and we should probably consider versioning it, in case we need to add/remove stuff in the future.
FYI: @libotony

@fabiofvc
Copy link
Contributor

@libotony
Copy link
Member

@davidecarpini Considering the deserialization for typed languages, what do you think making the field isVeWorldSupported required or normalizing it in the packing script?

@fabiofvc
Copy link
Contributor

@davidecarpini considering we have different Wallets (Sync, Sync2, VechainThor, VeWorld) why don't we move to a structure like:

walletSupported: {
  sync: true,
  sync2: true,
  vechainthor: true,
  veworld: true
}

?

For VeWorld POV is overkill, I know.
However, the AppHub itself could provide more data and a new filtering feature (by wallet).

@davidecarpini
Copy link
Member Author

@fabiofvc it's a good point, and normalizing the flag would be nice if in the future we aim to support multiple wallets. But I think we should keep just one wallet in the future, so it wouldn't make much sense in the long run.
BTW my POV on this flag is different, every newly accepted dapp should support veworld, it should be mandatory. This should be a retro compatibility flag that allows us to know which of these dapps is supporting veworld.
I see this flag being deprecated in the long long run, where we can request updates from the dapps to support the wallet or we will remove them from the app-hub.

@libotony I added all missing dapps flags (the 'false' ones) and added the flag as required in the validation script and in the readme

@libotony
Copy link
Member

@fabiofvc it's a good point, and normalizing the flag would be nice if in the future we aim to support multiple wallets. But I think we should keep just one wallet in the future, so it wouldn't make much sense in the long run. BTW my POV on this flag is different, every newly accepted dapp should support veworld, it should be mandatory. This should be a retro compatibility flag that allows us to know which of these dapps is supporting veworld. I see this flag being deprecated in the long long run, where we can request updates from the dapps to support the wallet or we will remove them from the app-hub.

@libotony I added all missing dapps flags (the 'false' ones) and added the flag as required in the validation script and in the readme

Agree, that wallet flag might be overkilling. My understanding is it was for veworld checking dApp's compatiabilty. It should be suggested that all dApps that supports the popular wallets.

@libotony libotony changed the title feat: add isVeWorldCompatible variable feat: add isVeWorldSupported variable Mar 19, 2024
@libotony libotony merged commit 37742a7 into vechain:master Mar 19, 2024
1 check failed
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.

4 participants