-
Notifications
You must be signed in to change notification settings - Fork 286
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
Follow-up improvements to sendbatch API, renew all #764
Conversation
thanks for the review @rithvikvibhu I addressed everything in 0e2eb39 |
lib/wallet/wallet.js
Outdated
// Enforce consensus limit per block at a maxmium | ||
finalizes++; | ||
if (finalizes > consensus.MAX_BLOCK_RENEWALS / 6) | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike other make*All
functions that cap at 100 (MAX_BLOCK_* / 6), this check happens after adding a finalize so batches are capped at 101. Will either have to pop here or move it to the start of loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks fixed.
@@ -2499,7 +2499,7 @@ class RPC extends RPCBase { | |||
_validateBatch(args, help, method) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To stay consistent with existing create* RPC method behaviour, _validateBatch
can maybe set default options.path = true
since all other RPC calls do this:
Lines 2170 to 2174 in 3fd74e0
if (!opts.name) { | |
const mtx = await wallet.createRevealAll({ | |
paths: true, | |
account: opts.account | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
Updated all bulk actions in Bob to use this, and everything works! kyokan/bob-wallet#546 |
- apply witness discount to multisig spends - fix "fallback" 2-of-3 script hash size value - account for large multisig scripts in script size varInt size
This will prevent the "Could not resolve preferred inputs" error that is thrown when a user sends a single reveal followed by a revealAll
c3bce92
to
4e9c7fc
Compare
replaces #708
follow-up to #686
This still fixes the two bugs listed in #708:
"Could not resolve preferred inputs"
fixed by usinggetUnspentCoin()
This fixes a bug in
sendbatch
that was just merged:sendbatch []
will not actually send an empty 1-in 1-out TX with no covenants :-(The fixes to the policy weight limit have been applied in a different way:
Current, unchanged behavior:
sendreveal
when executed with no arguments attempt to build a batch TX that reveals all bids for all names for all accounts in the wallet. Similar behavior forsendredeem
. If there are too many bids to reveal or redeem, these functions will fail by constructing a TX that is too big. This is not changed. We should probably consider deprecating this behavior in a future release, or those rpc commands entirely.New behavior:
sendbatch
andcreatebatch
were recently merged, and so we add new features to that command only. Any combination of these actions is valid:In these new cases, the wallet will add as many actions as it can to an MTX and stop just before it crosses the policy weight limit. That means if a user has a lot of bids to reveal, redeem, etc -- this command may be called multiple times until it finally stops sending anything.
"Renew all"
This is a function that has not been available in the hsd wallet before. There is still no
"sendrenewal
" with no arguments, but can be used withsendbatch
andcreatebatch
to renew all expiring names. "Expiring" is hard coded to mean:renewalWindow / 8
)A "renew all" batch will be limited not by policy weight limit, but much sooner below the consensus limit on renewals per block:
exports.MAX_BLOCK_RENEWALS / 6 = 100
UX Goal
An hsd-based wallet application can call
createbatch([['REVEAL'],['REDEEM'],['RENEW']])
at regular intervals (every block? every boot up? every 5 minutes...?) and display the batch to the user. If the user confirms, the batch will be sent, and thecreatebatch
command called again immediately. There may still be more stuff to do! Applications can request user approval for each batch, or send all the batch transactions needed in sequence until it's done everything it needs to do.Hosted wallet services with lots of user auction activity can call
sendbatch([['REVEAL'],['REDEEM'],['RENEW']])
in a try/catch.