All account types#529
Conversation
because iOS26 + Flutter no longer works properly on device
…s_keystone_accounts # Conflicts: # mobile-app/lib/providers/route_intent_providers.dart # mobile-app/lib/services/multisig_submission_service.dart # mobile-app/lib/v2/screens/accounts/add_account_menu_screen.dart # mobile-app/lib/v2/screens/accounts/create_account_screen.dart # quantus_sdk/lib/src/models/remote_config_model.dart
Review — All account typesReviewed the full diff against Bugs1. // account_utils.dart
int nextNonHardwareWalletIndex(List<Account> accounts) {
final used = getNonHardwareWalletIndices(accounts).toSet(); // non-hardware only
...
}
Repro: primary software wallet at index 0 → add a Keystone wallet (
A new wallet index must be unique across all wallet types. Suggest using 2. Multisig default threshold drops 2-of-2 → 1-of-2 // multisig_service.dart
static int defaultThreshold(int signerCount) {
if (signerCount <= 0) return 1;
return (signerCount * 2 / 3).round().clamp(1, signerCount);
}The old code enforced a minimum of 2 for any multisig with ≥2 signers. The new formula yields Concern — remote-config default flipsstatic const RemoteConfigModel defaults = RemoteConfigModel(
enableKeystoneHardwareWallet: true, // was false
enableHighSecurity: false, // was true
enableSwap: false, // was true
enableEncryptedAccount: false,
enableMultisig: true, // was false
...
);Several defaults flipped beyond what the description ("default off / staged rollout") implies:
These defaults govern first-launch/fallback behavior, and combined with Minor / nits
Looks good
Verdict: Request changesAddress #1 (wallet-index collision) before merge, decide on #2 (multisig threshold), and confirm the |
|
Re-reviewRe-reviewed after 1. Wallet-index collision — fixed. ✅ 2. Multisig threshold — acknowledged, intentional. Fine as a product decision; the user can still raise it in the UI. (For what it's worth, a 2-signer multisig defaulting to 1-of-2 is the one case I'd keep an eye on, but your call.) 3. Remote-config default flips — acknowledged, intentional. 👍 4. Local Keystone signature verification — agreed, non-issue. The chain already verifies and won't accept a bad signature; not worth duplicating. Remaining items are optional, non-blocking nits (active-account fallback could land on an encrypted account; hardcoded Verdict: Approve / LGTMThe blocking bug is resolved and the rest are deliberate decisions. Good to merge. |
| byWallet.putIfAbsent(a.walletIndex, () => []).add(a); | ||
| } | ||
|
|
||
| final walletIndexByAccountId = {for (final a in accounts) a.accountId: a.walletIndex}; |
There was a problem hiding this comment.
wow, first time seeing this way of declaring variable.
Not sure. if it's good because it's really hard to read by human.
| padding: const EdgeInsets.only(bottom: 12), | ||
| child: Text( | ||
| title, | ||
| style: context.themeText.smallParagraph?.copyWith(color: context.colors.textTertiary, fontSize: 12), |
There was a problem hiding this comment.
Should have used .details instead of smallParagraph. This way, we don't need to override font size.
| style: context.themeText.paragraph!.copyWith( | ||
| fontSize: 18, | ||
| fontWeight: FontWeight.w400, | ||
| color: colors.textPrimary, |
There was a problem hiding this comment.
unnecessary color and font weight.
| value: account.name, | ||
| onTap: () => _openNameEditor(context, ref, account), | ||
| ), | ||
| Divider(color: colors.toasterBackground, height: 1), |
There was a problem hiding this comment.
I think we have many of this repeated,
Divider(color: colors.toasterBackground, height: 1)
It might be worth it to have reusable divider. I remember I created that before not sure why it's not exist anymore.
| Text(l10n.keystoneScanScanning(_parts.length), style: text.detail?.copyWith(color: Colors.white70)), | ||
| if (AppConstants.debugHardwareWallet && !_submitting) ...[ | ||
| const SizedBox(height: 16), | ||
| TextButton( |
There was a problem hiding this comment.
why do we create new button on the fly here? we must use standard button component
| child: SafeArea( | ||
| child: Row( | ||
| children: [ | ||
| IconButton( |
There was a problem hiding this comment.
Same for Icon, we should use quantus icon button.
| final threshold = (signerCount * 0.7).round(); | ||
| final minThreshold = signerCount >= 2 ? 2 : 1; | ||
| return threshold.clamp(minThreshold, signerCount); | ||
| return (signerCount * 2 / 3).round().clamp(1, signerCount); |
There was a problem hiding this comment.
Why change this?
This change introduce bad default for when multisig only 2 people.
2 * 2 /3 = 1.3333, if we round it it will be 1. Because clamp now 1, the default will be 1. For 2 people multisig, having signer default to 1 when the signers are 2 people, basically useless.
There was a problem hiding this comment.
No this is what I wanted it to be, it's the formula I want to use, i think I also made tests for it
2: 1/2
3: 2/3
4: 3/4
5: 3/5
6: 4/6
7: 4/7
... etc
its just a default and a 2 people msig is very rate, who knows why they want to do that, could be they share an account (2 signers), or else they want 2/2... it's ok it can be changed by slider
Just the default needs to be reasonable
There was a problem hiding this comment.
ok these are the actual expected ones, all good
const expected = {1: 1, 2: 1, 3: 2, 4: 3, 5: 3, 6: 4, 7: 5, 8: 5, 9: 6, 10: 7};
There was a problem hiding this comment.
Okay, if that is what desired then make sense. Just still weird for 2 people to set signer minimum as 1, like it doesn't prevent anyone from spending it.
I dunno why you think it's rate maybe because you have seen people use it. But, from what I know in how people use bank account, couple can set need two signers to proceed balance.
Anyway, I don't really mind being 1 since it can be changed. Not worth fussing it.
dewabisma
left a comment
dewabisma
left a comment
There was a problem hiding this comment.
Okay, found some small parts need to be changed.
2 participants
Refactoring all account types and settings.
Included
enableMultisigremote flag)enableKeystoneHardwareWalletflag)enableEncryptedAccountremote flag)Notes
main: BIP-44 gap-limit discovery, global toast provider refactor, silent refresh on notifications, multisig remote flag.mobile-app/run_on_device.shto build/install/run on iOS 26 devices from the command line (Flutter's Xcode automation is currently broken on iOS 26).(Encrypted accounts were previously deferred in this description but are in fact included here, behind a flag.)