Go: Add `buildMemoRequiredTx` to txnbuild

[abuiles]

What problem does your feature solve?

This will make it easy for consumers to setup memoRequired in their accounts

What would you like to see?

A helper function which creates a valid transaction with the MANAGE_DATA operation that adds a data entry with the name config.memo_required

Refs #2329

[leighmcculloch]

If the idea of using data entries in a common way as described in SEP-18 and first demonstrated in SEP-29 really takes off and we end up with many more data entries to support, how will this function fit in a world where there is a collection of functions for configuring an account's data?

We could put the function in a sub-package of txnbuild so that all these functions are grouped together and the top-level of txnbuild doesn't get unwieldy.

Or, we could make a more general function that helps you convert a map[string]interface{} or a struct with tags into a transaction of data entries? Something akin to what is eluded to in the discussions in this comment https://github.com/stellar/stellar-protocol/pull/540#issuecomment-591117309 that summarize ideas @MisterTicot shared about converting data entries to JSON structures and back again, except doing it with Go structs.

[abuiles]

@leighmcculloch

we could make a more general function that helps you convert a map[string]interface{} or a struct with tags into a transaction of data entries? Something akin to what is eluded to in the discussions in this comment stellar/stellar-protocol#540 (comment) that summarize ideas @MisterTicot shared about converting data entries to JSON structures and back again, except doing it with Go structs.

Being able to turn a JSON to config fields sounds like a good idea, however, think it goes beyond the scope of what we are trying to achieve with this function.

If we do it, then it defies our main purpose here which is not surfacing to the end-user the fact that they have to set a field with config.memo_required: 1

We could put the function in a sub-package of txnbuild so that all these functions are grouped together and the top-level of txnbuild doesn't get unwieldy.

👍 sounds good, I think eventually we could put there the function that does JSON to config operations.

[ire-and-curses]

This is a good point about SEP-18. On the other hand there is no proposal yet for a standard way to implement "how an application takes a set of data entries and turns them into a tree data structure".

Making a SEP-specific helper with minimal args for SEP 29 seems valuable, because it's what you're going to look for when you want to implement SEP 29 support. Having a helper means you can be insulated from future changes to the SEP or doing something else non-standard.

Since this is such a trivial operation (set one field to a value), generalising it to take a map[string]interface{} seems bad to me at this time. At the point that SEP 18 (or the new SEP mentioned in the comment above) becomes a thing, this helper could be refactored internally if that made sense.

I do kinda like the idea of a subpackage for SEP support functions in general. We could plan to move e.g. the SEP 10 helpers and validators to such a package in due course.

[leighmcculloch]

I agree the map is a poor idea for communicating intent to our immediate use case. We also definitely don't need to implement JSON support for this here, that's not what I was suggesting.

One way to adopt the struct idea and to keep the intent clear is:

package txnbuilddata

// MemoRequired contains the fields required for SEP-29, that indicate if an account should only be sent payments with a memo.
type MemoRequired struct {
  MemoRequired bool `key:"config.memo_required"`
}

package txnbuilddata

// BuildDataTx builds a transaction with the value given serialized into the transaction as Manage Data operations. The value should be a struct. Fields on the value are only included if they are annotated with tags using the 'key' name. See MemoRequired as an example.
func BuildTx(v interface{}) error {

Use becomes:

tx, err := txnbuilddata.BuildTx(MemoRequired{true})
...

The main concern I have though is that we'll have txnbuild.BuildMemoRequiredTx, and then in 2 months txnbuild.BuildSecondFieldTx, then in 4 months, txnbuild.BuildThirdFieldTx, and so on. Putting these new functions into txnbuild/txnbuilddata would go a long way to separting them, and I agree I think the SEP-10 functions could move into something like txnbuild/txnbuildchallenge or similar.

[abuiles]

After thinking about it, the helper will return an operation, not a transaction.

If we were to return a transaction, we'll have to ask multiple data like the network, timebounds, and source account. Also this will restrict the transaction to this single operation, but what if you want to send other operations?

So to keep things simple, this will return the manageData operation which you can then use with a transaction like:

tx := txnbuild.Transaction{
        SourceAccount: source,
        Operations:    []txnbuild.Operation{&txnbuilddata.MemoRequired()},
        Timebounds:    txnbuild.NewTimeout(300),
        Network:       network.TestNetworkPassphrase,
    }

Also this keep things future-compatible in the scenario of building operations from a JSON. Let's make it easier to build the operations but then let the user choose how they want to shape the rest of the transaction.

[leighmcculloch]

So the function will be something like this? Seems very lightweight. Is it worth it for a limited number of users as a hold over till muxed accounts?

func NewManageDataMemoRequired(b bool) *ManageData {
  value := "0"
  if b {
    value = "1"
  }
  return &ManageData{
    Name: "config.memo_required",
    Value: value,
  }
}

[abuiles]

@leighmcculloch exactly - it will be a very simple function, I think is still worth adding it as reference and to avoid possible mistakes from users. Hopefully once muxed accounts arrived we can get rid of this and the memo_required check :)

[MisterTicot]

Looks like the issue is that we need a generalized form but we don't know
what it will look like yet. So I'd say that we need a SEP specifying the
general concept asap.

Some early ideas from my notes (I know it's not what you're discussing here
but maybe it can help taking a decision?)

Step 1. Map data types to specific encodings

One SEP to specify valid data types and their mandatory encodings. Example:

• _Strings_ must be encoded as UTF-8.
• _Numbers_ must be encoded as raw binary values.
• _Buffers_ must be encoded as raw binary values.
• _Float_ must be ....

Step 2. Each SEP declare the data type for its keys

That's the point of step one: SEPs don't need to define keys encoding but
only their data type. Easier to read & work with:

• config.multisig.coordinator must be a String.

Step 3. SDK to include a simplified schema that combine the whole key

set:

{
 config: {
  multisig: {
   coordinator: "string",
  },
  use_textmemo: "boolean"
 }
}

Step 4. SDK to include a standard account data tree encoding/decoding.

The AccountResponse object could provide a decoded data tree parameter.

A generic function could consume an edited data tree and return the
_Transaction_ that set an account data entries accordingly, based on the
diff between desired & current tree.

Also, at no additional cost (subroutine of the later): A function could
return an operation that set one specific key to the specified value.

The later looks like what Abuiles proposed and a tuned down version of it
could be implemented now, and then upgraded when the full thing is figured
out.

(The higher-level function in step 4 is what is being used in oc-multisig
so I think that's a realistic design)

I hope it helps.

Le mer. 11 mars 2020 à 22:01, Adolfo Builes [e-mail] a
écrit :

After thinking about it, the helper will return an operation, not a
transaction.

If we were to return a transaction, we'll have to ask multiple data like
the network, timebounds, and source account. Also this will restrict the
transaction to this single operation, but what if you want to send other
operations?

So to keep things simple, this will return the manageData operation which
you can then use with a transaction like:

tx := txnbuild.Transaction{
SourceAccount: source,
Operations: []txnbuild.Operation{&txnbuilddata.MemoRequired()},
Timebounds: txnbuild.NewTimeout(300),
Network: network.TestNetworkPassphrase,
}

Also this keep things future-compatible in the scenario of building
operations from a JSON. Let's make it easier to build the operations but
then let the user choose how they want to shape the rest of the transaction.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stellar/go/issues/2378#issuecomment-597876548, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AIWXOBEB7WMAJKNO4WNZHXDRG733LANCNFSM4LF4AAYQ
.

[leighmcculloch]

it will be a very simple function, I think is still worth adding it as reference

@abuiles I think if that's the goal for this to be a reference for how to build this operation successfully, sounds good. I think if there's anyway we can improve SEP-29 the doc so that it is clear there, then we should consider that too or maybe instead, but I don't feel strongly.

Something else we should consider is how many people will use this. If this will be used by a small number of exchanges, and be used only once by each one in a manual way meaning that on pubnet we can only expect to see this transaction maybe 10 times in total for the main use case, then writing code to support it might not be helpful to anyone.

[abuiles]

@leighmcculloch I explore a quick implementation above, but after giving it some more thought, I think you are right. The number of people using this might not be a lot and then this adds an extra burden on us for more code that we'll need to maintain and eventually remove once muxed accounts are a thing.

As for JSON to Operations, that's definitely out of the scope of this issue, let's keep the conversation in SEP18.

Additionally, we could add an extra check on @msfeldstein tool which checks for SEPs compatibility, to make sure the configuration was done correctly.

[leighmcculloch]

@abuiles: Adding this check to a tool sounds like a great idea I think.

So I'd say that we need a SEP specifying the general concept

@MisterTicot 👏 This sounds like a great idea I think.