[WIP] Add broadcast confirm sig logic#188
[WIP] Add broadcast confirm sig logic#188wesgraham wants to merge 9 commits intoFourthState:developfrom
Conversation
|
|
||
| // GetSignBytes returns the Keccak256 hash of the transaction. | ||
| func (msg ConfirmSigMsg) GetSignBytes() []byte { | ||
| return nil |
There was a problem hiding this comment.
I hope the sdk doesn't complain about this?
There was a problem hiding this comment.
Works for include deposit message right now so i'm assuming we won't have a problem?
|
Applying above changes. Also removing ability to broadcast arbitrary confirm sigs via flags for now as there is no natural mechanism for validating such signatures |
Codecov Report
@@ Coverage Diff @@
## develop #188 +/- ##
===========================================
- Coverage 52.02% 49.44% -2.59%
===========================================
Files 28 30 +2
Lines 1359 1430 +71
===========================================
Hits 707 707
- Misses 538 609 +71
Partials 114 114
Continue to review full report at Codecov.
|
| return includeCmd | ||
| } | ||
|
|
||
| var broadcastSigsCmd = &cobra.Command{ |
There was a problem hiding this comment.
Although the anteHandler for this msg will fail if the sig is wrong, we should do client side validation here before sending off the message.
hamdiallam
left a comment
hamdiallam
left a comment
There was a problem hiding this comment.
Shaping up great! The flow looks correct to me.
| } | ||
|
|
||
| // validate inputs/confirmation signatures if not a fee utxo or deposit utxo | ||
| if !input.IsDeposit() && !input.IsFee() { |
There was a problem hiding this comment.
If the input is a deposit or fee, we should not return sdk.Result{}. It should be an error. There are no confirm signature for these kinds of inputs. Otherwise this will get sent to the handler
| return ErrInvalidSignature("invalid signature provided").Result() | ||
| } | ||
|
|
||
| _, ok := ds.GetOutput(ctx, input.Position) |
There was a problem hiding this comment.
do we need this if we're already doing GetTxWithPoisition?
| if err != nil { | ||
| return ErrInvalidInput(fmt.Sprintf("failed to retrieve exit information on input, %v", in.Position)).Result() | ||
| } else if exited { | ||
| return ErrExitedInput(fmt.Sprintf("a parent of the input has exited. Position: %v", in.Position)).Result() |
There was a problem hiding this comment.
can we make this message better w.r.t confirm signatures? like 'cannot include confirm signatures of a input transaction that has exited..."
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| ) | ||
|
|
||
| // NewSpendHandler sets the inputs of a spend msg to spent and creates new |
| return func(ctx sdk.Context, msg sdk.Msg) sdk.Result { | ||
| confirmSigMsg, ok := msg.(msgs.ConfirmSigMsg) | ||
| if !ok { | ||
| panic("Msg does not implement SpendMsg") |
|
|
||
| for i := 0; i < len(tx1.Transaction.Inputs); i++ { | ||
| // TODO: Is TxIndex sufficient? Also should likely check output index? | ||
| if tx1.Transaction.Inputs[i].Position.TxIndex != confirmSigMsg.Input1.Position.TxIndex { |
There was a problem hiding this comment.
can we add an Equals method to Input and simply to if tx1.Transaction.Input[i].Position.Equals(...) { do the thing}.
Typically do not want to use continue. Should wrap the state updates within the if block for when it should happen. Makes it easier to read.
| panic("Msg does not implement SpendMsg") | ||
| } | ||
|
|
||
| /* Get tx, update tx data with confirm sigs, and store updated tx object */ |
There was a problem hiding this comment.
Instead of doing single comments like this. Can you break that up and comment the places where each point occurs
|
|
||
| // SpendMsg implements the RLP interface through `Transaction` | ||
| type ConfirmSigMsg struct { | ||
| Input1 plasma.Input |
There was a problem hiding this comment.
do we have to restrict to 2 inputs. Why not include as many inputs as someone likes. []Input
3 participants
(Fixes #148)
Adds the ability to broadcast confirm signatures without spending inputs. Requesting feedback on signature requirements, confirmSigMsg struct, and information to be stored in datastore. Tests pending.