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

Initial Xelis integration #1084

Open
wants to merge 21 commits into
base: staging
Choose a base branch
from
Open

Conversation

Tritonn204
Copy link

Barring reported bugs/issues in our focus group, this is the final state of the initial implemented codebase for Xelis in Stack Wallet.

Copy link
Collaborator

@julian-CStack julian-CStack left a comment

Choose a reason for hiding this comment

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

I didn't comment on all the build_all*.sh scripts but the others need to be changed back so libepiccash builds correctly.

There also seem to be some image changes that can be removed form the PR. Mostly SVGs and the addition of a png that shouldn't be there.

The dart code all looks to be good and integrated well. I only scanned quickly and didn't run anything yet though.

@@ -137,6 +141,11 @@ class TxData {
return null;
}

String? get getOtherData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This getter isn't really needed I think?

Copy link
Author

Choose a reason for hiding this comment

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

It's not necessary at this moment, but it was done to enable passing Xelis asset info to the TX call. If I end up using the sub-wallet model for Xelis tokens when they're more fleshed out, this may end up being removed, but for the inheritance structure I have sketched out in my mind, passing the asset through the extra data this way seems to be the cleanest.

Unless the value can be accessed without any getter? I remember running into issues accessing the field without this getter, but I may have missed a way to do this without a new method, being new to Dart. LMK if this is what you meant

Copy link
Collaborator

Choose a reason for hiding this comment

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

otherData isn't private so you can just access it without a getter. Dart strings are immutable, and otherData is marked final so it cannot be modified once set when the instance of TxData is created.

There are some other models where we do an "otherData" String but those are database models (tables essentially) where there are limitations on what and how things are serialized to be written to disk. The otherData in that case is a bit of a dirty hack to store dynamic/unstructured data, usually by encoding it to a Map, then doing jsonEncode/jsonDecode when storing or fetching.

The TxData class is never written to disk at all so that hack isn't required there. I'm not sure what is stored in the otherData field but if its modelled on the hack mentioned above, I would do something else if possible.

@Tritonn204 Tritonn204 changed the base branch from main to staging February 26, 2025 22:02
@@ -12,7 +12,7 @@ mkdir -p build
(cd ../../crypto_plugins/flutter_libepiccash/scripts/windows && ./build_all.sh )
(cd ../../crypto_plugins/flutter_liblelantus/scripts/windows && ./build_all.sh )
set_rust_to_1720
(cd ../../crypto_plugins/frostdart/scripts/windows && ./build_all.sh )
(cd ../../crypto_plugins/xelis_flutter/scripts/windows && ./build_all.sh )
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be reverted

@@ -10,7 +10,7 @@ mkdir -p build
(cd ../../crypto_plugins/flutter_libepiccash/scripts/windows && ./build_all.sh )
(cd ../../crypto_plugins/flutter_liblelantus/scripts/windows && ./build_all.sh )
set_rust_to_1720
(cd ../../crypto_plugins/frostdart/scripts/windows && ./build_all.sh )
(cd ../../crypto_plugins/xelis_flutter/scripts/windows && ./build_all.sh )
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be reverted

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.

2 participants