CIP-0030 Suggestions

[mkazlauskas]

Paginate
...
if a wallet is modified between paginated calls that this will change the pagination, e.g. some results skipped or showing up multiple times but otherwise the wallet must respect the pagination order.

To avoid this issue we could change API functions with pagination to return an async iterator instead of Promise.

PaginateError
{maxSize} is the maximum size for pagination and if the dApp tries to request pages outside of this boundary this error is thrown.

Is the dApp supposed to try to call the method with {limit: Number.POSITIVE_INFINITY} to figure out max size? I think it's an odd API and if we keep the concept of pagination there should be a way to get the limit without an error.

type TransactionUnspentOutput = cbor<transaction_unspent_output>
...
api.getUtxos(amount: cbor<value> = undefined, paginate: Paginate = undefined): Promise<TransactionUnspentOutput[] | undefined>

I initially thought that TransactionUnspentOutput wasn't cbor, the definition could use better consistency: I think we should either define all types as cbor<[type]> or list all type definitions under Data Types.

api.getBalance(): Promise<cbor<value>>

Does it include rewards and deposit? Need to make this explicit, as using these funds requires certificates in the transaction.

api.getUsedAddresses(paginate: Paginate = undefined): Promise<cbor<address>[]>
api.getUnusedAddresses(): Promise<cbor<address>[]>

There will be different types of wallets. Some might be single address and not have any unused addresses. Given this API, dApp will probably expect to always get some unused addresses, which might not be the case. The dApp itself can figure out if it was used or not. I suggest to consolidate these to api.getAddresses(paginate: Paginate = undefined): Promise<cbor<address>[]>.

The wallet is free to return thereject with TxSendError

@rooooooooob

[rooooooooob]

To avoid this issue we could change API functions with pagination to return an async iterator instead of Promise.

Would the wallet do something like ordering all utxos and then have the returned generator remember the last returned one and query the wallet for the next one in whatever arbitrary ordering they chose yielding that until it's there are none? I'm not primarily a javascript dev (more C++/Rust) and haven't worked with this feature before so I just want to be sure this would be easily doable and solve the issues with the current API.

Is the dApp supposed to try to call the method with {limit: Number.POSITIVE_INFINITY} to figure out max size? I think it's an odd API and if we keep the concept of pagination there should be a way to get the limit without an error.

The idea was they would pick a limit based on their own need like 10 or 50 or whatever and repeatedly call it with increasing page numbers until PaginateError is returned. I guess if you wanted to know how many pages there would be you could use infinity. To be honest I'm not really familiar with typical paging APIs so I had hoped someone would point out better ways during the original PR review if possible. I guess the above generator approach wouldn't let you figure out how many it would return in total either though?

I initially thought that TransactionUnspentOutput wasn't cbor, the definition could use better consistency: I think we should either define all types as cbor<[type]> or list all type definitions under Data Types.

Do you mean including all the types that are defined via cbor<T> explicitly? The problem with this is that they are just composed of other types and including those too would be a lot of types. Plus I think it's best to refer to directly to the binary spec. All other types are listed under Data Types so I don't think it's a problem.

Does it include rewards and deposit? Need to make this explicit, as using these funds requires certificates in the transaction.

This was discussed in #88 here but we never finalized any changes. Perhaps it's time to continue that discussion that was started there and get to a final solution. @KtorZ @SebastienGllmt @alessandrokonrad

There will be different types of wallets. Some might be single address and not have any unused addresses. Given this API, dApp will probably expect to always get some unused addresses, which might not be the case. The dApp itself can figure out if it was used or not. I suggest to consolidate these to api.getAddresses(paginate: Paginate = undefined): Promise<cbor

[]>.

Do we expect the dApp to filter them all to see if they've ever been used on-chain before? I think people would be using the two endpoints (used and unused) for very different purposes. We could change the API description to mention that they shouldn't expect there to always be addresses if that would help. We should see what others think though.

[mkazlauskas]

Would the wallet do something like ordering all utxos

I think we should be thinking in terms of sets of utxo (unordered).

and then have the returned generator remember the last returned one and query the wallet for the next one in whatever arbitrary ordering they chose yielding that until it's there are none?

I think we shouldn't assume any specific wallet implementation or data source. It could already have all utxo in memory, could be fetching it from some server, or loading from indexeddb. Generator implementation would depend on wallet internals, but from API user point of view it would be a generator of the most recent snapshot of the utxo set.

I'm not primarily a javascript dev (more C++/Rust) and haven't worked with this feature before so I just want to be sure this would be easily doable and solve the issues with the current API.

Not yielding duplicates should be trivial to implement regardless of where the wallet is sourcing the utxo (checking by tx id + index). The wallets can also not yield a utxo once it's spent, even if it's spent after returning the generator. There might still be a situation where a utxo that was yielded is spent before the generator completes. There could be an error for that.

For this specific function of api.getUtxos, I actually suggest just api.getUtxos(amount: cbor<value> = undefined): Promise<TransactionUnspentOutput[]>. What was the idea behind pagination for this one in the first place?

The idea was they would pick a limit based on their own need like 10 or 50 or whatever and repeatedly call it with increasing page numbers until PaginateError is returned. I guess if you wanted to know how many pages there would be you could use infinity.

I thought the PaginateError error was for a too large page size requested. I don't think there's much value in having an error for when there are no more pages, as it can be inferred from an empty array result. I actually suggest to just remove this error.

To be honest I'm not really familiar with typical paging APIs so I had hoped someone would point out better ways during the original PR review if possible.

A common pattern is to return something like {items: Item[], numTotalItems: number}.

I guess the above generator approach wouldn't let you figure out how many it would return in total either though?

Yes, you wouldn't know the total number of items, but you can stop taking them when you got enough.

Do you mean including all the types that are defined via cbor explicitly?

That's one option, but I suggest getUtxos is declared as: api.getUtxos(amount: cbor<value> = undefined, paginate: Paginate = undefined): Promise<cbor<transaction_unspent_output>[] | undefined>, not using the TransactionUnspentOutput type declared above for consistency (it's the only type declared this way).

This was discussed in #88 here but we never finalized any changes. Perhaps it's time to continue that discussion that was started there and get to a final solution. @KtorZ @SebastienGllmt @alessandrokonrad

The discussion seems to be about total vs available balance/utxo. I think it's ok for the connector to always return available balance/utxo (would be great to document it though). My question was about what this balance consists of. I suggest returning something like that: { utxo: cbor<value>; rewards: Lovelace; keyDeposit: Lovelace; poolDeposit: Loverlace; }

Do we expect the dApp to filter them all to see if they've ever been used on-chain before? I think people would be using the two endpoints (used and unused) for very different purposes. We could change the API description to mention that they shouldn't expect there to always be addresses if that would help. We should see what others think though.

Yes I was thinking the dApp would filter them, but now that you mention very different purposes I no longer think that consolidating them is a good idea. Changing the description would be a good improvement.

[rooooooooob]

Thanks for all the feedback.

I think we should be thinking in terms of sets of utxo (unordered).

I wasn't saying it should be in the spec, I was just thinking of a possible way to implement it that would solve both issues with the previous approach, those being that you might skip utxos if the set changed mid-iteration (e.g. you are on page 5 and something in pages 1-4 was spent so now everything shifts and you might miss a utxo) and that maybe new utxos will be skipped in a similar way. Having some kind of ordering was just the first idea that came to mind for me in order to implement it in a way that solves both of those issues - I didn't mean to have it sound like I was mandating it as the only way.

For this specific function of api.getUtxos, I actually suggest just api.getUtxos(amount: cbor = undefined): Promise<TransactionUnspentOutput[]>. What was the idea behind pagination for this one in the first place?

We might have been overly conservative of which endpoints might need pagination since it is optional functionality anyway. If you don't pass in a Paginate instance (it default to undefined) then you get back everything.

That's one option, but I suggest getUtxos is declared as: api.getUtxos(amount: cbor = undefined, paginate: Paginate = undefined): Promise<cbor<transaction_unspent_output>[] | undefined>, not using the TransactionUnspentOutput type declared above for consistency (it's the only type declared this way).

The problem is that transaction_unspent_output isn't in the binary spec. It's just a convenience type that we've made up since using just the spec's input type doesn't give you the value which makes it useless for figuring out input selection without consulting the ledger. Or are you talking about adding it in the cbor<T> header as a raw cddl definition and then referring to it with cbor<transaction_unspent_output> directly everywhere else?

[mkazlauskas]

Thanks for all the feedback.

Thanks for considering my suggestions.

Having some kind of ordering was just the first idea that came to mind for me in order to implement it in a way that solves both of those issues

If we think about the issues separately, duplicate items issue is really just some complexity that needs to be handled either by the wallets or by the dapps. Both can do it, but I think this API should aim to make dapp development easy.

I have an alternative suggestion. Instead of optional pagination argument, how about a separate API method that you would use like this:

const pageSize = 10;
const { numPages, getPage } = await api.paginateUsedAddresses(pageSize);
for (let page=0; page<numPages; page++) {
  const addressesPage = await getPage(page); // an array of addresses with length up to pageSize
}

We might have been overly conservative of which endpoints might need pagination since it is optional functionality anyway.

I think amount argument is sufficient. I'm wondering if I'm missing some use case.

The problem is that transaction_unspent_output isn't in the binary spec.

Oh, I see. I misunderstood the purpose of that type def. Thanks for an explanation.