Ridvan batch finish #47
Conversation
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
| continue; | ||
|
|
||
| const output = new Output(); | ||
| output.address = coin.address; |
There was a problem hiding this comment.
Just to make sure I understand: we are sending REDEEM and REGISTER transactions to the same address that sent the REVEAL, correct?
There was a problem hiding this comment.
This is a consensus requirement for REGISTER (see lib/covenants/rules.js:1282) but not for REDEEMs. @rozaydin can you confirm that the previous behavior for normal REDEEMs was to send the REDEEM output to the same address and not to a new address? I can imagine wanting to send the coins to a different address since REDEEMs are spendable. In any case not a blocking change
| }); | ||
|
|
||
| assert.deepStrictEqual(wallet2Finish.errorMessages, []); | ||
| assert.equal(wallet2Finish.processedFinishes.length, 2); // one redeem one finish |
There was a problem hiding this comment.
It doesn't seem like these tests check if the correct bids were redeemed/regsitered, what do you think? Blockchain consensus will prevent actual errors, but if we have a problem in transaction construction and mess up which was the redeem and which was the register, then our REVEAL outputs will be locked in the bad, pending transaction and we might get a "could not resolve preferred inputs" error.
There was a problem hiding this comment.
I have updated the tests to assert for REDEEM and REGISTER values, please let me know if this would be enough or should we add additional checks ?
lib/wallet/http.js
Outdated
| // Create Batch Finish (Redeem or Register) | ||
| this.post('/wallet/:id/batch/finish', async (req, res) => { | ||
| const valid = Validator.fromRequest(req); | ||
| let names = valid.array('names'); |
There was a problem hiding this comment.
names seems misleading to me because it's not the same as the names variable in batch opens/reveals. Maybe const finishRequests = valid.array('finishRequests')?
lib/wallet/http.js
Outdated
| let errorMessages = []; | ||
| const uniqueNames = []; | ||
|
|
||
| names.forEach(({name, data}) => { |
There was a problem hiding this comment.
Do you think it's worth abstracting this common logic from the batch bids code?
| continue; | ||
|
|
||
| const output = new Output(); | ||
| output.address = coin.address; |
There was a problem hiding this comment.
This is a consensus requirement for REGISTER (see lib/covenants/rules.js:1282) but not for REDEEMs. @rozaydin can you confirm that the previous behavior for normal REDEEMs was to send the REDEEM output to the same address and not to a new address? I can imagine wanting to send the coins to a different address since REDEEMs are spendable. In any case not a blocking change
| output.covenant.pushHash(nameHash); | ||
| output.covenant.pushU32(ns.height); | ||
|
|
||
| if (prevout.equals(ns.owner)) { // Winner, does REGISTER |
There was a problem hiding this comment.
When does ns.owner get set?
- Are we sure that ns is not set on every tree interval, when the name tree gets committed? Or is it up to date with every block?
- What happens if there is a redeem/register right on the exact close_block of an auction? Will
ns.owneralready point to the correct output?
|
|
||
| finishes.forEach((element) => { | ||
| const {output, outpoint} = element; | ||
| mtx.addOutpoint(outpoint); |
There was a problem hiding this comment.
For my understanding, what is the difference between this line and the one below it?
There was a problem hiding this comment.
outpoint contains the previous output (prevout) (which forms the inputs i believe) where as the output is the outputs that we are creating with this transaction... per my understanding
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Signed-off-by: rozaydin <ridvan@namebase.io>
Batch Finish hsd implementation