213 provide hd wallet implementation #363
Conversation
…m Wallet.java Need to rework signing to get addresses.
Thus removing the unneeded utxosupplier value in Wallet.
| * @param utxoSupplier | ||
| * @return | ||
| */ | ||
| public List<Account> getSignersForTransaction(Transaction tx, WalletUtxoSupplier utxoSupplier) { |
There was a problem hiding this comment.
Should this method also take into account the following potential location for "signers"?"
-
Transaction Required Signers (https://github.com/IntersectMBO/cardano-ledger/blob/master/eras/babbage/impl/cddl-files/babbage.cddl#L66) - Could include Staking credentials also
-
Transaction Inputs (covered here I think)
-
Collateral Inputs (https://github.com/IntersectMBO/cardano-ledger/blob/master/eras/babbage/impl/cddl-files/babbage.cddl#L65)
-
Certificates (https://github.com/IntersectMBO/cardano-ledger/blob/master/eras/babbage/impl/cddl-files/babbage.cddl#L58) - This one I think could require signing with Staking Credentials
There was a problem hiding this comment.
@gavinharris-dev This is good feedback. I think instead of returning List<Account>, it can return List<TxSigner> or just TxSigner by composing multiple signers.
I agree that we should try to resolve all possible signers for the above scenarios. There are a few additional governance-related signers introduced in the Conway era where we need to sign using Drep credentials.
As the Wallet API will be compatible with the QuickTx API, additional signers can also be added in a standard way.
| private final UtxoService utxoService; | ||
| @Setter | ||
| private Wallet wallet; | ||
| private static final int INDEX_SEARCH_RANGE = 20; // according to specifications |
There was a problem hiding this comment.
Would it be possible for this go into a configuration property somewhere? I have seen wallets with 100s of addresses which is super annoying to try supporting; but if we were as users able to configure this it might be good?
I wonder if there would be a way for Dev's to retain a list of Derivation Paths that are known good ones? A lot of the multi address wallets out there have < 5 active addresses, but a heap of addresses that were used once (and no longer have any UTxOs linked).
There was a problem hiding this comment.
Makes sense.
We need to revisit this. Apart from the current implementation, which follows the HD wallet specification (scanning 20 unused addresses), we can provide an additional implementation or extend the current one with additional configuration properties. As you mentioned, one of these could be:
- Specific derivation paths provided by the application. The application needs to store these derivation paths in persistent storage and provide them to this API.
@gavinharris-dev Thanks for your comments / feedback. This PR is still in draft, and the current plan is to finalize it after the Conway HF, as it will impact some existing key APIs. So, please feel free to provide any feedback. |
|
Changed the sender wallet handling to return null instead of throwing an exception in Tx.java to allow flexibility in error management. Also, removed redundant 'amounts' field and its initialization from AbstractTx.java for a cleaner codebase.
Move the transaction building logic into a dedicated private method in `TxBuilderContext`. Refactor `QuickTxBuilder` to use the new build method and streamline the signing process, eliminating redundant code and improving code readability.
Refactor Wallet methods and variables for better naming consistency. Remove unused methods and annotations. Add new test for scanning specific indexes in DefaultWalletUtxoSupplier. Update existing tests to align with the refactored method names and structures.
|
|
I have done some minor cleanup and changes to this PR. It seems the core functionalities are working fine. I think it's ready to be merged. Though there are a few enhancements required before the release, we can handle them in separate PRs. Can you please have a quick look before we merge this? |
|
I have added a separate issue to track future enhancements of the Wallet api |
| // * @param utxoSupplier | ||
| // * @return | ||
| // */ | ||
| // public List<Account> getSignersForTransaction(Transaction tx, WalletUtxoSupplier utxoSupplier) { |
There was a problem hiding this comment.
This function isn't needed? Or will it added later again?
There was a problem hiding this comment.
@Kammerlo This is not needed currently, but I have kept it commented out for now in case it is required for any edge scenarios.
Implementation of a first version HD wallet wrapper.