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

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

Closed
1 of 3 tasks
2opremio opened this issue May 7, 2021 · 15 comments
Closed
1 of 3 tasks
Assignees

Comments

@2opremio
Copy link
Contributor

2opremio commented May 7, 2021

Following the description of stellar/stellar-protocol#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)

  • Querying transactions by account
  • Querying operations by account
  • Querying effects by account? (we may want to stick to G-addresses for effects)

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

  • Responding with Operation objects (source account and payment/merge/clawback destination)
  • Responding with Transaction objects (source account)
  • Responding with Effects?

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

@2opremio
Copy link
Contributor Author

2opremio commented May 7, 2021

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
Copy link
Member

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
Copy link
Contributor

Shaptic commented May 7, 2021

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:

  • (1ish) would be "If a user provides a G-address, Horizon behaves as it already does.", and
  • (4ish) would be "If a user provides an M-address, Horizon turns it into a G-address and (1ish) applies."

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
Copy link
Contributor

tomerweller commented May 10, 2021

(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) 👎 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) 👍 Effectively current behavior. Doesn't make any assumptions about how muxed accounts are used.

(4) 👎 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 2opremio self-assigned this May 12, 2021
@2opremio
Copy link
Contributor Author

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

@2opremio
Copy link
Contributor Author

2opremio commented May 12, 2021

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
Copy link
Member

leighmcculloch commented May 12, 2021

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 2opremio changed the title Horizon: support querying by Muxed Account and incorporate Muxed accounts in resposnses Horizon: support querying by Muxed Account and incorporate Muxed accounts in responses May 12, 2021
@2opremio
Copy link
Contributor Author

2opremio commented May 12, 2021

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

@leighmcculloch sure!

@2opremio
Copy link
Contributor Author

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:

  • For querying, we will pick option (3) (G-strkey globbing) and nothing else for now. I will however spend some time thinking about what schema changes would be required for (2) (strict M-strkey querying)
  • For responses there was already quorum on adding _muxed and _muxed_id suffixes, as described in SEP-23. So we will do that.

@2opremio
Copy link
Contributor Author

2opremio commented May 17, 2021

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
Copy link
Contributor Author

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
Copy link
Member

leighmcculloch commented May 17, 2021

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
Copy link
Contributor

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:

if memoBytes, err := memoBytes(row.TxEnvelope); err != nil {

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

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

No branches or pull requests

4 participants