stellar / go

Stellar's public monorepo of go code
https://stellar.org/developers
Apache License 2.0
1.3k stars 499 forks source link

Horizon: support querying by Muxed Account and incorporate Muxed accounts in responses #3591

Closed 2opremio closed 3 years ago

2opremio commented 3 years ago

Following the description of https://github.com/stellar/stellar-protocol/issues/617 we should add support for Muxed Accounts in:

  1. All account-parameterized requests
  2. Responses

With respect to requests ~we should support Muxed Accounts when:~ (edit: we decided to not do this for now)

With respect to responses, we should support Muxed Accounts when:

Note: this description may not be complete yet, I will be incorporating new tasks while going through the API and DB.

2opremio commented 3 years ago

The first thing we should clarify/design is how Horizon should respond based on whether it receives an M-acount or or a G-account as a query parameter. Depending on what we decide we could end up with a different DB schema.

There are a bunch of approaches which could be followed:

  1. Be strict about G-addreses:

    • If the user provides a G-address (lets call it GADDR ), she strictly means that G-address explicitly. Responses should only contain data related to GADDR . In particular, M-addresses with the same underlying GADDR should be discarded.
  2. Be strict about M-addresses:

    • If the user provides an M-address (lets call it MADDR , with underlying G-address GADDR), she strictly means that M-address explicitly. Responses should only contain data related to that M-address. Other M-addresses with the same underlying GADDR as MADDR should be discarded. In addition, data related to GADDR (i.e. the Ed25519 variant of MADDR, without an Id) should also be discarded.
  3. Glob G-addresess:

    • If the user provides a G-address (lets call it GADDR ), she means any Ed2551 or Med25519 account with the same Ed25519 data as GADDR. In particular the response should include all the M-addresses with the same Ed25519 value as GADDR, regardless of their ID. And, of course, GADDR should be included.
  4. Glob M-addresses:

    • If the user provides an M-address (lets call it MADDR , with underlying G-address GADDR). she means any Ed2551 or Med25519 account with the same Ed25519 data as GADDR. In particular the response should include all the M-addresses with the same Ed25519 value as GADDR, regardless of their ID. And, of course, GADDR should be included.

I would choose (2) and (3): be strict about M-addresses and glob G-addresses. Globbing G-addresses would keep the API backwards compatible with respect to the filtering sematics (although we still need to decide how to present M-addresses).

We could also make it configurable at query-time, but that would complicate the API and (maybe) the DB schema.

Thoughts?

leighmcculloch commented 3 years ago

My instinct is the same as you would choose, (2) and (3).

I think they represent the two major perspectives the data would be looked up: strict M address lookup (2) when wallets using an M address are looking up data, and glob G address lookup (3) when the pooled account owner is streaming data for all their virtual accounts.

Regarding 2 – This is what I'd expect if I use an M address. If we don't support strict M address lookup, I think it would be safer to not support M address lookup and require the G address to be used, otherwise the API would be confusing, for me at least.

Regarding 3 – Agree for same reason you mentioned, backwards compatibility.

Regarding 1 – As long as the transaction responses returned include the muxed information I'm not convinced 1 is necessary. It might be useful for pooled account owners identifying payments that came in without muxed information, to identify lost payments.

Regarding 4 – I don't have an opinion on 4. I think 4 isn't necessary because clients can decode the G address from the M address and so query the G address themselves, but maybe Horizon will choose to provide that query, but it seems like an optimization to me.

Shaptic commented 3 years ago

Answering this first requires a fundamental understanding of the scope of Horizon's role within muxed accounts now (which described above) but also in the future (unknown?). Are we (eventually) providing a de-facto custodial implementation? Will it ultimately be for state-ful endpoints or just historical endpoints? As much as I like the idea of not having x slightly-different custodial implementations out there in the world for every exchange that wants it (out of the interest of not reinventing the wheel x times), this is still a pretty big burden for Horizon to bear, resource- and complexity-wise.

My vote would be to keep Horizon as pure as Core in this regard (i.e. Core doesn't "understand" muxed accounts; they're an off-chain convention as far as they're concerned), meaning a vote for (1)-ish and (4)-ish. By "ish" I mean something even simpler than what they're offering above:

This would be the lowest-impact implementation, right? No DB changes, just transform any M-address in a request to its underlying G-address and move on, then shove the M-address into the response at the end (or don't, because the caller should already know which M-address was queried for).

I do think (2) and (3) makes sense for a "true" implementation, but maybe we should wait until there's a clear need for us to take on this role before making the investment? We can just offer barebones support (i.e. just accept the M as if it's a G) for now, and let the consumers of the feature deal with the actual muxing as they'd like.

I realize this goes against the grain of the deployment plan a little bit (i.e. we aren't actually offering any mux-enabled features, like filtering operations by muxed account), but it's also a lower-impact way to test the waters of how wide-spread this feature will ultimately be.

tomerweller commented 3 years ago

(1) :-1: Seems dangerous as it can obfuscate critical information for an account. For example, if I subscribe to listen to payments on my G account then senders can "hide" themselves from this feed by introducing a mock muxed id.

(2) :-1: Might be beneficial for block explorers or other third parties interested in showing activity history. However, that feed can be considered incomplete as it doesn't capture internal payments within the pooled account operator's system. Probably not relevant for the pooled account operators themselves - they are watching all activity in their pooled accounts (using 3) and most likely indexing it anyway for the relevant sub-accounts.

(3) :+1: Effectively current behavior. Doesn't make any assumptions about how muxed accounts are used.

(4) :-1: Sounds dangerous as it presents more information than what the user asked for. If that's what clients are interested in doing they can simply extract the G address and query for that.

In summary. I vote for 3 alone - doing nothing (querying wise). Maybe down the line we can introduce 2 if there's sufficient ecosystem demand

2opremio commented 3 years ago

Thanks all for adding your comments. We will discuss it in the Horizon meeting and I will come back with what we decided!

2opremio commented 3 years ago

Now, regarding Muxed accounts in responses, my plan is to add new fields with a _muxed suffix (other name suggestions are welcome). The reason to keep existing fields as is, is backwards compatibility.

The field won't be present if the account used isn't muxed.

For instance, here's an /operations response with the new fields:

{
  "_links": {
    "self": {
      "href": "https://horizon.stellar.org/operations/?cursor=\u0026limit=10\u0026order=asc"
    },
    "next": {
      "href": "https://horizon.stellar.org/operations/?cursor=33818572492801\u0026limit=10\u0026order=asc"
    },
    "prev": {
      "href": "https://horizon.stellar.org/operations/?cursor=12884905985\u0026limit=10\u0026order=desc"
    }
  },
  "_embedded": {
    "records": [
      {
        "_links": {
          "self": {
            "href": "https://horizon.stellar.org/operations/12884905985"
          },
          "transaction": {
            "href": "https://horizon.stellar.org/transactions/3389e9f0f1a65f19736cacf544c2e825313e8447f569233bb8db39aa607c8889"
          },
          "effects": {
            "href": "https://horizon.stellar.org/operations/12884905985/effects"
          },
          "succeeds": {
            "href": "https://horizon.stellar.org/effects?order=desc\u0026cursor=12884905985"
          },
          "precedes": {
            "href": "https://horizon.stellar.org/effects?order=asc\u0026cursor=12884905985"
          }
        },
        "id": "12884905985",
        "paging_token": "12884905985",
        "transaction_successful": true,
        "source_account": "GA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVSGZ",
       "source_account_muxed": "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJUAAAAAAAAAAAACJUQ",
        "type": "create_account",
        "type_i": 0,
        "created_at": "2015-09-30T17:15:54Z",
        "transaction_hash": "3389e9f0f1a65f19736cacf544c2e825313e8447f569233bb8db39aa607c8889",
        "starting_balance": "20.0000000",
        "funder": "GAAZI4TCR3TY5OJHCTJC2A4QSY6CJWJH5IAJTGKIN2ER7LBNVKOCCWN7",
        "account": "GALPCCZN4YXA3YMJHKL6CVIECKPLJJCTVMSNYWBTKJW4K5HQLYLDMZTB"
      },
      {
        "_links": {
          "self": {
            "href": "https://horizon.stellar.org/operations/12884905986"
          },
          "transaction": {
            "href": "https://horizon.stellar.org/transactions/3389e9f0f1a65f19736cacf544c2e825313e8447f569233bb8db39aa607c8889"
          },
          "effects": {
            "href": "https://horizon.stellar.org/operations/12884905986/effects"
          },
          "succeeds": {
            "href": "https://horizon.stellar.org/effects?order=desc\u0026cursor=12884905986"
          },
          "precedes": {
            "href": "https://horizon.stellar.org/effects?order=asc\u0026cursor=12884905986"
          }
        },
        "id": "12884905986",
        "paging_token": "12884905986",
        "transaction_successful": true,
        "source_account": "GA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVSGZ",
        "source_account_muxed": "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJUAAAAAAAAAAAACJUQ",
        "type": "payment",
        "type_i": 1,
        "created_at": "2015-09-30T17:15:54Z",
        "transaction_hash": "3389e9f0f1a65f19736cacf544c2e825313e8447f569233bb8db39aa607c8889",
        "asset_type": "native",
        "from": "GAAZI4TCR3TY5OJHCTJC2A4QSY6CJWJH5IAJTGKIN2ER7LBNVKOCCWN7",
        "to": "GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY",
        "to_muxed": "MAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSAAAAAAAAAAE2LP26",
        "amount": "99999999959.9999700"
      },

In this case all the accounts are multiplexed, but, as I mentioned, a missing _muxed field would indicate that the account use isn't multiplexed (i.e. Ed25519 type and not Med25519).

Thoughts?

leighmcculloch commented 3 years ago

my plan is to add new fields with a _muxed suffix

Looks good to me. It looks the same as the SEP-23 proposal for Horizon responses.

Can you also include the _muxed_id which is also mentioned in the SEP? It will allow systems using only the JSON data to extract the iD simply. If it is added, can it be added as a string type? Even though the value is an integer ID it's an int64 and JavaScript only formally supports 32bit. Also, we have discussed adding non-integer IDs so strings would lend themselves to that extension in the future.

2opremio commented 3 years ago

Can you also include the _muxed_id which is also mentioned in the SEP? It will allow systems using

@leighmcculloch sure!

2opremio commented 3 years ago

We discussed this in the Horizon meeting and decided to keep it simple and avoid schema changes (which would be required for querying by M-strkey).

Thus, the conclusion so far is that:

2opremio commented 3 years ago

The only remaining point so far is, do we care about Muxed accounts for effects?

I am not sure it adds a lot of value. If we decide to do it, I propose to incorporate the _muxed and _muxed_id suffixes like we will do with the operation details:

That would be something like:

{
  "_links": {
    "self": {
      "href": "https://horizon.stellar.org/effects?cursor=\u0026limit=10\u0026order=asc"
    },
    "next": {
      "href": "https://horizon.stellar.org/effects?cursor=33676838572034-1\u0026limit=10\u0026order=asc"
    },
    "prev": {
      "href": "https://horizon.stellar.org/effects?cursor=12884905985-1\u0026limit=10\u0026order=desc"
    }
  },
  "_embedded": {
    "records": [
      {
        "_links": {
          "operation": {
            "href": "https://horizon.stellar.org/operations/12884905986"
          },
          "succeeds": {
            "href": "https://horizon.stellar.org/effects?order=desc\u0026cursor=12884905986-1"
          },
          "precedes": {
            "href": "https://horizon.stellar.org/effects?order=asc\u0026cursor=12884905986-1"
          }
        },
        "id": "0000000012884905986-0000000001",
        "paging_token": "12884905986-1",
        "account": "GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY",
        "acount_muxed": "MAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSAAAAAAAAAAE2LP26",
        "account_muxed_id": 1234,
        "type": "account_credited",
        "type_i": 2,
        "created_at": "2015-09-30T17:15:54Z",
        "asset_type": "native",
        "amount": "99999999959.9999700"
      },
2opremio commented 3 years ago

In addition, I don't think it's going to be possible to add source_account_muxed and source_account_muxed_id for transactions without changing the schema.

Unlike with operations, for which we have a schema-free JSON column for the details, the transactions table would need to be modified in order to support the new fields.

leighmcculloch commented 3 years ago

do we care about Muxed accounts for effects?

I think we do, simply for the fact that once this change is rolled out for other endpoints we're making a statement that when _muxed is present, the address was muxed, and when not, it wasn't. It will be very unintuitive that a missing _muxed on effects doesn't also signal that the address wasn't muxed.

For this reason, I think the new sidecar fields need to apply everywhere an account field exists today and in the future. If we think that is not a realistic expectation maybe we need to rethink the response format.

tomerweller commented 3 years ago

In addition, I don't think it's going to be possible to add source_account_muxed and source_account_muxed_id for transactions without changing the schema.

The schema already contains the transaction envelope xdr, which should contain all the information required.

Looks like the envelope is already being used to extract memos here: https://github.com/stellar/go/blob/6d45e627f4ab14d111199dc6d1d2fa8f80ea1276/services/horizon/internal/resourceadapter/transaction.go#L45

We can move the envelope decoding outside of the memoBytes() function and reuse it for muxed accounts as well.

With that said, decoding every transaction envelope on the fly might have a performance impact.

2opremio commented 3 years ago

With that said, decoding every transaction envelope on the fly might have a performance impact.

I implicitly discarded doing that for performance, but maybe it's acceptable? (I doubt it)

2opremio commented 3 years ago

For the record, we ended up changing the schema and adding columns for the muxed accounts