-
Notifications
You must be signed in to change notification settings - Fork 73
Add transactions multi-signing. #76
Add transactions multi-signing. #76
Conversation
* Add `MultiSign` and `SetSigners` methods for transactions to be signed from multi-signing. * Add additional `MultiSignable` interface for the `BaseTx` and implement it. * Fix structure used by `SIGNER_LIST_SET` and `multi-signing` * Update memo struct to use the additional structure as memo item instead of embedded structure * Add missing `temBAD_WEIGHT` error code * Add `Exception` to `CommandError` for the better error message
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.
Hi, lots of changes in a single commit. Would be good to break it into separate PR's in case you break anyone's code.
data/format.go
Outdated
HP_INNER_NODE HashPrefix = 0x4D494E00 // 'MIN' inner node in tree | ||
HP_LEDGER_MASTER HashPrefix = 0x4C575200 // 'LWR' ledger master data for signing (probably should have been LGR!) | ||
HP_TRANSACTION_SIGN HashPrefix = 0x53545800 // 'STX' inner transaction to sign | ||
HP_TRANSACTION_MILTISIGN HashPrefix = 0x534D5400 // 'SMT' inner transaction to multi-sign |
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.
typo
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.
Do you mean to split to MILTI_SIGN
?
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.
s/MILTISIGN/MULTISIGN/
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.
Right, fixed.
@@ -133,12 +133,16 @@ type Escrow struct { | |||
DestinationNode *NodeIndex `json:",omitempty"` | |||
} | |||
|
|||
type SignerEntry struct { | |||
type SignerEntryItem struct { |
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.
Have you got an example tx or ledge entry from mainnet which demonstrates this structure?
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.
- Doc: https://xrpl.org/signerlistset.html#example-signerlistset-json
- Here is the tesnet example (unfortunately I don't have the same for the mainnet): https://testnet.xrpl.org/transactions/01A6D400C0F114543A3BE26708F9E3CBBFDDDF40000E1D5C85E3F0DDFE5EC9C1/raw
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.
This is a ledger entry and that example transaction is a Payment and does not modify the ledger entry. You need to find a SignerListSet transaction which changes the ledger entry to confirm that this change is necessary. I suspect it isn't, but happy to be proved wrong :-)
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.
My bad, that was the proof for the Signer
and SignerItem
.
And here is the proof for the SignerListSet
: https://testnet.xrpl.org/transactions/05AADAF31482E06241870D93D32FDC5E59414D7A9D60754D1C1DEC5ACFDF3860/raw
data/memo.go
Outdated
@@ -1,11 +1,13 @@ | |||
package data | |||
|
|||
type MemoItem struct { |
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.
Why the change?
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.
With the previous embedded struct, it was convenient if you read the transaction, but if you create a transaction and you need to define the memo you would need to copy the embedded struct definition every time you do it.
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.
I have been doing this:
memos := make(data.Memos, 1)
memos[0].Memo.MemoData = data.VariableLength(quoteData)
Not particularly elegant, but does your change break this code?
Irrespective of this, it's a change being slipped into a PR for another problem, so best to break it out into a separate issue/PR.
After that happy to merge.
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.
The proposed changes might be a breaking change depending on the way the memo is initialized. If it's initialized in the way you do - not breaching, but if in the way where you provide the embedded struct, it's breaking.
That change is removed from that PR and will be moved to another.
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.
PR: #77
websockets/commands.go
Outdated
Name string `json:"error"` | ||
Code int `json:"error_code"` | ||
Message string `json:"error_message"` | ||
Exception string `json:"error_exception"` |
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.
Is this a new response field from somewhere? Docs?
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.
Actually, I've not found it in the spec. It was found in the responses from the testnet
.
You can try that code snippet (with updated code):
func main() {
host := "wss://s.altnet.rippletest.net:51233/"
remote, err := websockets.NewRemote(host)
panicIfErr(err)
amount, err := data.NewAmount("100000") // 0.1 XRP tokens
tx := data.Payment{
Amount: *amount,
TxBase: data.TxBase{
TransactionType: data.PAYMENT,
},
}
// submit the transaction
_, err = remote.Submit(&tx)
fmt.Printf("%s\n", err)
}
func panicIfErr(err error) {
if err != nil {
panic(err)
}
}
The output:
invalidTransaction 0 gFID: uncommon type out of range 0
Raw response.
{"error":"invalidTransaction","error_exception":"gFID: uncommon type out of range 0","id":1,"request":{"command":"submit","id":1,"tx_blob":"12000024000000006140000000000186A06880000000000000008114000000000000000000000000000000000000000083140000000000000000000000000000000000000000"},"status":"error","type":"response"}
The output if you run it from the master:
invalidTransaction 0
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.
This should be in a separate PR too. Just in case anyone needs to check the commit history to easily see where changes have been added.
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.
Removed from that PR. And will be in a different PR.
Add transaction multi-signing.
MultiSign
andSetSigners
methods for transactions to be signed from multi-signing.MultiSignable
interface for theBaseTx
and implement it.SIGNER_LIST_SET
andmulti-signing
temBAD_WEIGHT
error codeResolves https://github.com/rubblelabs/ripple/issues/40
Resolves https://github.com/rubblelabs/ripple/issues/44
Resolves https://github.com/rubblelabs/ripple/issues/75