Skip to content
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

txnbuild: enables multiple signatures #1198

Merged
merged 7 commits into from
Apr 30, 2019
Merged

Conversation

debnil
Copy link
Contributor

@debnil debnil commented Apr 26, 2019

This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every Operation type have a different source account than its Transaction. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness.

@@ -43,6 +44,5 @@ func (p *Payment) BuildXDR() (xdr.Operation, error) {
Asset: xdrAsset,
}
body, err := xdr.NewOperationBody(opType, xdrOp)

return xdr.Operation{Body: body}, errors.Wrap(err, "failed to build XDR OperationBody")
return xdr.Operation{SourceAccount: p.SourceAccount, Body: body}, errors.Wrap(err, "failed to build XDR OperationBody")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we set the new field p.SourceAccount?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nvm. It looks like we expect the caller to set it

Copy link
Contributor

@howardtw howardtw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I left a nit and some questions

txnbuild/helpers_test.go Outdated Show resolved Hide resolved
expected := "AAAAAODcbeFyXKxmUWK1L6znNbKKIkPkHRJNbLktcKPqLnLFAAAAZAAiII0AAAAbAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAABAAAAAH4RyzTWNfXhqwLUoCw91aWkZtgIzY8SAVkIPc0uFVmYAAAAAAAAAAAF9eEAAAAAAAAAAAHqLnLFAAAAQNcGQpjNOFCLf9eEmobN+H8SNoDH/jMrfEFPX8kM212ST+TGfirEdXH77GJXvaWplfGKmE3B+UDwLuYLwO+KbQQ="
assert.Equal(t, expected, received, "Base 64 XDR should match")
}

func TestPaymentFromDiffSourceAcct(t *testing.T) {
kp0 := newKeypair0()
txSourceAccount := makeTestAccount(kp0, "9605939170639898")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me understand how we get this seq number? Can we have a comment explaining why this number?

}

received := buildSignEncode(t, tx, kp0, kp1)
expected := "AAAAAODcbeFyXKxmUWK1L6znNbKKIkPkHRJNbLktcKPqLnLFAAAAZAAiII0AAAAbAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAEAAAAAJcrx2g/Hbs/ohF5CVFG7B5JJSJR+OqDKzDGK7dKHZH4AAAABAAAAAH4RyzTWNfXhqwLUoCw91aWkZtgIzY8SAVkIPc0uFVmYAAAAAAAAAAAF9eEAAAAAAAAAAALqLnLFAAAAQHzYkZeogiHztanqRvrXXxiNShH/Zf5EUjgabrb6wwgX1eOUBRjp5J92qq8s/o1B1sxrMNiPpViAq40tD/yGfwjSh2R+AAAAQNVC6YLIbAnFs3G/rdf7IxrWYFOxjOKUSZsN0q1Bm/MXk+7ydhcCbYBgq+VGa6eZf8BckgIdAtDI8VNWPoTyhAM="
Copy link
Contributor

@howardtw howardtw Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we know that this is the expected XDR? I just copied and pasted it to the XDR viewer and verified it's correct 😄But I'm not sure whether it's necessary to compare received with a non human readable string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it needs to be confirmed by running the transaction once in the wild (and seeing that it produces/does what was intended). After that, the purpose of the test is mostly to guarantee that this doesn't break with future changes. To be fair, the style of the tests was already here before this PR. :)

Copy link
Member

@ire-and-curses ire-and-curses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! It's a big improvement. :) Would be awesome if you would

  1. Switch to using Account in the struct rather than xdr.AccountId
  2. Confirm that the transaction produced is valid in the wild
  3. Extend this approach to all the operations

Destination string
Amount string
Asset Asset
SourceAccount *xdr.AccountId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I think this would be better as an Account type, as is down with the same field in Transaction.

  1. It matches Transaction.SourceAccount
  2. Caller can pass an account directly after retrieving it from horizonclient
  3. Caller doesn't have to fiddle with the xdr construction, which is kind of awkward as it's a setter

Copy link
Member

@ire-and-curses ire-and-curses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah looks good, thanks! Just a few small comments. Happy for you to merge after you look at them.

Gopkg.lock Outdated
@@ -453,6 +453,14 @@
pruneopts = "T"
revision = "efea85cb9a06f1d4f661649f6beb5fd04597a366"

[[projects]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this shouldn't be in this PR. @bartekn fixed it recently though so maybe you just need to merge from master?

assert.NoError(t, err)

return
return txeBase64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to return explicitly here, then you should remove the named return in the function definition.

return ManageSellOffer{
func CreateOfferOp(selling, buying Asset, amount, price string, sourceAccount ...Account) (ManageSellOffer, error) {
if len(sourceAccount) > 1 {
return ManageSellOffer{}, errors.New("multiple source accounts")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this error a bit clearer? How about "an offer can't have multiple source accounts"?

@@ -9,51 +9,73 @@ import (

//CreateOfferOp returns a ManageSellOffer operation to create a new offer, by
// setting the OfferID to "0".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's modify the docstring to indicate that sourceAccount is optional.

if len(sourceAccount) == 1 {
offer.SourceAccount = sourceAccount[0]
}
return offer, nil
}

//UpdateOfferOp returns a ManageSellOffer operation to update an offer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same docstring comment here

return ManageSellOffer{
func UpdateOfferOp(selling, buying Asset, amount, price string, offerID int64, sourceAccount ...Account) (ManageSellOffer, error) {
if len(sourceAccount) > 1 {
return ManageSellOffer{}, errors.New("multiple source accounts")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same error comment here

if len(sourceAccount) == 1 {
offer.SourceAccount = sourceAccount[0]
}
return offer, nil
}

//DeleteOfferOp returns a ManageSellOffer operation to delete an offer, by
// setting the Amount to "0".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same docstring comment here

// It turns out Stellar core doesn't care about any of these fields except the amount.
// However, Horizon will reject ManageSellOffer if it is missing fields.
// Horizon will also reject if Buying == Selling.
// Therefore unfortunately we have to make up some dummy values here.
return ManageSellOffer{
if len(sourceAccount) > 1 {
return ManageSellOffer{}, errors.New("multiple source accounts")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same error comment here

@@ -87,7 +88,9 @@ func (so *SetOptions) BuildXDR() (xdr.Operation, error) {
opType := xdr.OperationTypeSetOptions
body, err := xdr.NewOperationBody(opType, so.xdrOp)

return xdr.Operation{Body: body}, errors.Wrap(err, "failed to build XDR OperationBody")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we check err here?

@debnil debnil merged commit 47ee6ef into stellar:master Apr 30, 2019
@debnil debnil deleted the multiple-signers branch April 30, 2019 20:57
ire-and-curses added a commit that referenced this pull request May 2, 2019
* horizonclient/txnbuild README fixes (#1210)

* fix client links in top-level readme

* update clients README

* mark old client deprecated in godoc short summary

* fix code of conduct, standardise example

* fix code of conduct/contributing links

* txnbuild: enables multiple signatures (#1198)

This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every `Operation` type have a different source account than its `Transaction`. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness.

* root repo changelog links to sub-projects (#1214)

* keystore: add update-keys endpoints to spec (#1215)

We need an endpoint to update the encrypted seed when users forget their
passwords.

* move keystore to exp to fix build (#1223)

* Add minimal files to fix build (#1225)

* changelog for txnbuild 1.1
poliha pushed a commit that referenced this pull request May 3, 2019
* horizonclient/txnbuild README fixes (#1210)

* fix client links in top-level readme

* update clients README

* mark old client deprecated in godoc short summary

* fix code of conduct, standardise example

* fix code of conduct/contributing links

* txnbuild: enables multiple signatures (#1198)

This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every `Operation` type have a different source account than its `Transaction`. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness.

* root repo changelog links to sub-projects (#1214)

* keystore: add update-keys endpoints to spec (#1215)

We need an endpoint to update the encrypted seed when users forget their
passwords.

* move keystore to exp to fix build (#1223)

* Add minimal files to fix build (#1225)

* changelog for txnbuild 1.1
poliha added a commit that referenced this pull request May 3, 2019
* horizonclient/txnbuild README fixes (#1210)

* fix client links in top-level readme

* update clients README

* mark old client deprecated in godoc short summary

* fix code of conduct, standardise example

* fix code of conduct/contributing links

* txnbuild: enables multiple signatures (#1198)

This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every `Operation` type have a different source account than its `Transaction`. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness.

* root repo changelog links to sub-projects (#1214)

* keystore: add update-keys endpoints to spec (#1215)

We need an endpoint to update the encrypted seed when users forget their
passwords.

* move keystore to exp to fix build (#1223)

* Add minimal files to fix build (#1225)

* exp/ticker: Orderbook data support (#1193)

* exp/ticker: update partial market query to also output base/counter asset types

* exp/ticker: add functions to scrape orderbook data

* exp/ticker: update orderbookstats param names to match market standards

* exp/ticker: create orderbookstats database model and migrations

* exp/ticker: consolidate upsert logic and implement orderbookstats upsert

* exp/ticker: create CLI command to ingest orderbooks

* exp/ticker: update horizon SDK path

* exp/ticker: add orderbook stats to market.json generator

* exp/ticker: add separate query to retrieve relevant markets

* exp/ticker: add orderbook stats to graphql interface

* exp/ticker: add Docker support for orderbook ingestion

* exp/ticker: fix partial aggregated market query + add tests

* exp/ticker: fix globall aggregated market query + add tests

* exp/ticker: update ticker binary link and fix crontab comment

* exp/ticker: fix issue that would enable inf values to be stored in db

* exp/ticker: ensure only store valid orderbook entries are stored

* exp/ticker: format gql/static/bindata.go to prevent CI errors

* exp/ticker: add updated bindata with pg 9.5-compatible migrations

* exp/ticker: ensure markets with 0 orderbook entries can be handled by tickerdb

* exp/ticker: remove unnecessary commas from graphql schema

* exp/ticker: consolidate bid and ask positions on field names

* exp/ticker: simplify logic for creating orderbook requests

* exp/ticker: fix how bid and ask volumes are calculated

* exp/ticker: allow negative spread values

* exp/ticker: fix close_time fallback on markets query

* exp/ticker: add page size limit to orderbook requests

* Make PR template a bit more helpful. (#1238)

* Make PR template a bit more helpful.

Clean up some typos, and point to an example docs folder in the go repo
rather than just the external docs hosted on our site.

* Fix typo.

* Agh, formatting.

* horizonclient: update Fund method (#1213)

* update client.Fund

* update change log

* fix space

* horizonclient: add more documentation (#1226)

* Remove old stream method

* more comments

* fix format

* split comment

* horizonclient: Set default HTTP client (#1228)

* set default HTTP client

* changes from review

* changelog for txnbuild 1.1 (#1231)

* horizonclient/txnbuild README fixes (#1210)

* fix client links in top-level readme

* update clients README

* mark old client deprecated in godoc short summary

* fix code of conduct, standardise example

* fix code of conduct/contributing links

* txnbuild: enables multiple signatures (#1198)

This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every `Operation` type have a different source account than its `Transaction`. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness.

* root repo changelog links to sub-projects (#1214)

* keystore: add update-keys endpoints to spec (#1215)

We need an endpoint to update the encrypted seed when users forget their
passwords.

* move keystore to exp to fix build (#1223)

* Add minimal files to fix build (#1225)

* changelog for txnbuild 1.1

* update change log (#1233)

* horizonclient: add version (#1229)

* add package version

* fix go fmt

* clients/horizonclient: support dynamic effects (#1217)

* update effects in protocols/horizon

* update client interface

* update tests

* update changelogs

* fix typo
bartekn pushed a commit that referenced this pull request May 6, 2019
* horizonclient/txnbuild README fixes (#1210)

* fix client links in top-level readme

* update clients README

* mark old client deprecated in godoc short summary

* fix code of conduct, standardise example

* fix code of conduct/contributing links

* txnbuild: enables multiple signatures (#1198)

This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every `Operation` type have a different source account than its `Transaction`. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness.

* root repo changelog links to sub-projects (#1214)

* keystore: add update-keys endpoints to spec (#1215)

We need an endpoint to update the encrypted seed when users forget their
passwords.

* move keystore to exp to fix build (#1223)

* Add minimal files to fix build (#1225)

* changelog for txnbuild 1.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants