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

CIP30: getUtxos null instead of undefined #181

Merged
merged 1 commit into from
May 11, 2022
Merged

Conversation

SebastienGllmt
Copy link
Contributor

@SebastienGllmt SebastienGllmt commented Dec 23, 2021

Currently CIP30 defines the getUtxos endpoint should return undefined if the requested amount can't be satisfied

The problem is that browser message passing uses JSON and undefined is not valid JSON. This can easily introduce subtle bugs (some libraries silently convert undefined to null) and it's needless complexity when we could just define the spec to return null instead.

Strictly speaking this is a breaking change, but I'm not sure if it will affect app at the moment.

Another option would be to return an error instead of null which I think also makes sense

@SebastienGllmt SebastienGllmt self-assigned this Dec 23, 2021
@alessandrokonrad
Copy link
Contributor

I'm not even sure if undefined or null make even sense here, because the return type is TransactionUnspentOutput[]. If no utxos were found by filtering by certain assets, it should return an empty array imo.

@SebastienGllmt
Copy link
Contributor Author

It's not filtering by an asset -- it's filtering by an amount

Honestly though I think taking in an argument list for everything in CIP30 was a mistake. We should have made function only have a single parameter -- a JS object. It would have make backward/forward compatibility much easier -- especially for functions like this where people may want to add different filters, etc. in the future but can't anymore because of the optional parameter

@alessandrokonrad
Copy link
Contributor

It's not filtering by an asset -- it's filtering by an amount

Yeah, but as you say it's filtering, so normally when I apply a filter on an array I always get back an array again regardless of the result.

@SebastienGllmt
Copy link
Contributor Author

Returning an empty array would only make sense if this was element-wise filtering though. Otherwise you wouldn't be able to differentiate between a wallet with no UTXOs and a wallet with UTXOs but just not enough to sum to the value you requested

@mkazlauskas
Copy link

Returning an empty array would only make sense if this was element-wise filtering though. Otherwise you wouldn't be able to differentiate between a wallet with no UTXOs and a wallet with UTXOs but just not enough to sum to the value you requested

Agree. I'm wondering what is the intended use case for this filtering. If it's for input selection then this interface is not sufficient and should be redesigned in my opinion (see this comment).

vsubhuman added a commit to vsubhuman/CIPs that referenced this pull request Feb 4, 2022
Replaced undefined with null to be compatible with cardano-foundation#181
KtorZ pushed a commit that referenced this pull request Apr 5, 2022
* Adding `getCollateralUtxos` function

* Changed undefined return to null

Replaced undefined with null to be compatible with #181

* Updated the spec to rename the function and change the arguments to an object

* Fixing wording

* Update README.md

* Tried to clarify the description a bit more
@KtorZ KtorZ merged commit c6781db into master May 11, 2022
@KtorZ KtorZ deleted the cip30-undefined-to-null branch May 11, 2022 06:56
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.

7 participants