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

ABI decoder does not allow zero-length arrays #497

Assignees
Labels
days new-bug Bug report that needs triage Team Scytale

Comments

@jeapostrophe
Copy link

jeapostrophe commented Jan 14, 2022

https://github.com/algorand/js-algorand-sdk/blob/develop/src/abi/abi_type.ts#L31

but

https://github.com/algorandfoundation/ARCs/blob/main/ARCs/arc-0004.md#types --- "<type>[<N>]: A fixed-length array of length N, where N >= 0. type can be any other type."

([1-9][\d]*) should just be [\d]+

@jeapostrophe jeapostrophe added the new-bug Bug report that needs triage label Jan 14, 2022
@jeapostrophe jeapostrophe changed the title ABI does not allow zero-length arrays ABI decoder does not allow zero-length arrays Jan 14, 2022
@jasonpaulos
Copy link
Contributor

jasonpaulos commented Jan 14, 2022

We can't use [\d]+ with the code as is, since it would make things like uint64[0000000001] appear as a valid type. And we don't want this because method signatures need to have consistent hashes -- the ARC should probably clarify this.

But we could modify the regex in another way to support a single 0. Though it's worth noting this type is essentially a noop since you can't store anything in a 0 length static array.

@jasonpaulos
Copy link
Contributor

Just made this: fabrice102/ARCs#2

@jeapostrophe
Copy link
Author

Good point!

https://en.wikipedia.org/wiki/Robustness_principle --- I think you should accept uint64[00001] but never generate it =)

@jeapostrophe
Copy link
Author

This is still not fixed. You should change the regexp:

const staticArrayRegexp = /^([a-z\d[\](),]+)\[(0|[1-9][\d]*)]$/;

@algoanne algoanne added the days label Nov 17, 2022
@algoanne
Copy link

need to look at the other SDKs as well, probably same issue & fix.

@jeapostrophe
Copy link
Author

jeapostrophe commented Nov 29, 2022

Thank you! We should also reference #698

@ahangsu
Copy link
Contributor

ahangsu commented Nov 29, 2022

oops github is acting smart here and closed the ticket 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment