Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ARC56: Extended App Description #258
base: main
Are you sure you want to change the base?
ARC56: Extended App Description #258
Changes from 24 commits
ef5f338
6beb9f9
a18a968
1265709
1b19b76
45a4f6a
96bf578
8c983a1
a4c523e
f6a62f0
358a65b
d9046e1
9a5e445
ef47b89
8e3499b
b02c3c1
2a5eb6f
cebfc7d
e6115f0
fd8fe2a
89a6c0c
35fd3fc
8572596
ac0a9b1
05cd0d5
ab956a7
22372b2
8ee7866
f82a2fa
ad5d033
e540d92
7b3982b
f30b5f6
59edfa8
7f5816a
9908f86
faf5f4b
506acbd
d8d4c46
6bc830c
da85edf
3ef93fc
e577cfb
58a133d
aa79e86
35a19b3
3476dad
1e31ba0
19c6269
017c6e3
d58be41
9ba13f1
20369ed
027462f
12e3a53
8e2db8d
9a3f705
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description should probably be added to the networks key above otherwise you don't see it in intellisense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be any harm if we include the
TMPL_
prefix? The less assumptions (within the bounds of practicality) the better, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if we use it for code generation then you’d have to strip the TMPL so we have to cater for it somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If certain tooling expects that as a prefix, that seems like the place to check for and strip that prefix, yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shrug could go either way, I guess it's fair that this could be more general purpose and then the generators are clever enough to detect and strip TMPL_ when emitting code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My rationale omitting the TMPL_ here and forcing tooling to use it is to make it easier to implement safely. For example, if ARC56 allowed anything as the variable name and the tooling didn't have to add
TMPL_
, it would need to check all the variable names against all opcodes. Otherwise an ARC56 file could have a template variable namedbyte
which subsequently overwrites allbyte
opcodes which is not the intended effect of template variables.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It occurs to me that this format requires property order to be maintained to be able to map them to the underlying tuple type. Are we comfortable that will always be the case across all languages?
https://www.rfc-editor.org/rfc/rfc7159 says (emphasis mine):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this a choice for the HLL. The struct have to have a specific order for ABI encoding, so the HLL can either implement an ordered struct or not expose ways of iterating keys/values (like in TEALScript)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this spec determines the format of the json file? If the json file is an object and the programming language parsing it doesn’t implemented ordered iteration of object keys you are hosed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right ok I see what you are saying. So I think it should be
StructFields = { fieldName: string, type: ABIType | StructFields }[]
to preserve order as an array, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spot on I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does StorageKey have keyType? Isn't this for singular, known values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I figured it's still useful to know the type for things like explorers and frontends
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's intended to indicate the encoding of
key
? MaybekeyEncoding
would be clearer? I don't think it benefits from consistency of naming withStorageMap.keyType
since that seems of a different purpose - to indicate what sort of value to encode to combine withStorageMap.key
in order to retrieve a value. WhereasStorageKey.key
is already encoded and you may want to know how to interpret that value. So they're kind of opposites?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm a bit confused about what
keyType
means?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, I think I see.
So what should this and the
key
value be for a string value like we had in arc-32?Needing to have key as a base64 reduces the human readability of these files... I wonder if keyType should allow for a raw string to be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be the equivalent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then that doesn't let explorers etc. know that the key is actually a (non-ARC4) string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be confusing to have some keys be ASCII encoded and some keys be base64, thus everything base64 makes the most sense to me. I'm not sure if human readability is all that important. If you want to make it human readable just through it into Lora (once supported).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Human readable is a nice feature though - makes it easy to see things at a glance, I've certainly found that useful in the past.
Regardless if that's supported or not, it probably makes sense to support a keyType of
utf-8
, which would either mean non-base64 or base64bytes
, but representing a UTF-8 string so the likes of Lora can show it properly.Also, similarly we should probably have a key type of avm_address or similar to denote when you use the public key bytes of an address directly (i.e. non-ARC4 encoding).