RPC add method listsinceblock,getunusedaddress and change getnewaddress#4
Conversation
ghost
commented
Oct 15, 2019
ghost
commented
- Change getnewaddress method so that it will always generate a new address
- Add listsinceblock method
…ress Add listsinceblock method
Fix block height condition
dangershony
left a comment
dangershony
left a comment
There was a problem hiding this comment.
Generally looks good perhaps also add the method to the list of RPC methods
| [ActionName("getnewaddress")] | ||
| [ActionDescription("Returns a new wallet address for receiving payments.")] | ||
| public NewAddressModel GetNewAddress(string account, string addressType) | ||
| public NewAddressModel GetNewAddress(string account = "", string addressType = "") |
There was a problem hiding this comment.
Should we have an optional parameter to indicate the flow generating new or asking for a current unused?
There was a problem hiding this comment.
Or perhaps this should be a setting value that defaults to false?
There was a problem hiding this comment.
We should keep this compatible with how other Bitcoin implementations work. Adding additional parameters and/or configuration settings would require the integrating party to do additional work. That will be a hurdle for exchanges to adopt the coins because every new exchange has to write custom code or has to have custom knowledge to support our coins.
There was a problem hiding this comment.
The problem this is an HD wallet and just generating a new address comes with consequences (one needs to maintain a standard gap for recovery to work)
That's why I propose a settings flag to invert this, alternatively we can pass a mandatory field in recovery that takes a gap then an exchange that does recovery can juts put a very big number there
There was a problem hiding this comment.
Settings flag already in place: "walletaddressbuffer" defaults to 20
|
|
||
| int blockHeight = transactionData.BlockHeight ?? 0; | ||
|
|
||
| if (block != null && blockHeight < this.ChainIndexer.GetHeader(block?.GetHash()).Height) |
There was a problem hiding this comment.
This is waistfull, you can instead vat the method above GetBlock also fetch the header
| } | ||
| } | ||
|
|
||
| model.LastBlock = this.ConsensusManager.Tip.HashBlock; |
There was a problem hiding this comment.
It's better practice to use the tip of ChainIndexer
| [ActionName("listsinceblock")] | ||
| [ActionDescription("Get all transactions in blocks since block 'blockhash', or all transactions if omitted.")] | ||
| public async Task<ListSinceBlockModel> ListSinceBlockAsync(string blockHash, int targetConfirmations = 1) | ||
| { |
There was a problem hiding this comment.
Did you base the logic on bitcoin core by any chance?
There was a problem hiding this comment.
This should be the same logic that is used in bitcoin core
| HdAccount walletAccount = wallet.GetAccount(accountReference.AccountName); | ||
| HdAddress hdAddress = walletAccount.CreateAddresses(this.Network, 1).Single(); | ||
|
|
||
| string base58Address = hdAddress.Address; |
There was a problem hiding this comment.
ah hang on I think this is missing some functionality here
There was a problem hiding this comment.
Brilliant looks much better actually
dangershony
left a comment
dangershony
left a comment
There was a problem hiding this comment.
Discussion on going in discord
| int index = this.context.ActionContext.ActionDescriptor.Parameters.IndexOf(parameter); | ||
| var parameters = (JArray)req["params"]; | ||
|
|
||
| if (parameters == null) |
There was a problem hiding this comment.
ok excellent I think a fix was made in stratis repo a well
dangershony
changed the title
Add Transaction Details Dialog
1 participant
