Initial Xelis integration #1084
Conversation
julian-CStack
left a comment
julian-CStack
left a comment
There was a problem hiding this comment.
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.
lib/wallets/models/tx_data.dart
Outdated
| return null; | ||
| } | ||
|
|
||
| String? get getOtherData { |
There was a problem hiding this comment.
This getter isn't really needed I think?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
scripts/windows/build_all_duo.sh
Outdated
| (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 ) |
There was a problem hiding this comment.
the frostdart line should not be removed IIRC. Unless it gets built automatically now?
There was a problem hiding this comment.
Frostdart on Windows is still broken actually, cause the frostdart windows folders don't have any .sh files.
I think because of this, the build instructions linked to in README say to manually build frostdart yourself after the fact on Windows, while this line in the build scripts for Windows only serves to make the build script error out at the end
There was a problem hiding this comment.
Broken in the original devops**
| int get minConfirms => 1; | ||
|
|
||
| @override | ||
| bool get torSupport => true; |
There was a problem hiding this comment.
This should be set to false I believe unless I missed where the Tor proxy info is passed along to the xelis client?
| import 'test_stellar_node_connection.dart'; | ||
| import 'tor_plain_net_option_enum.dart'; | ||
|
|
||
| import 'package:logging/logging.dart' as std_logging; |
There was a problem hiding this comment.
the logging package does not appear to be used or included in the pub spec template?
scripts/windows/build_all_duo.sh
Outdated
| (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 ) |
There was a problem hiding this comment.
the frostdart line should not be removed IIRC. Unless it gets built automatically now?
…e-compliant xelis lib
2 participants
Barring reported bugs/issues in our focus group, this is the final state of the initial implemented codebase for Xelis in Stack Wallet.