-
Notifications
You must be signed in to change notification settings - Fork 109
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?
Conversation
ARCs/arc-draft_extended_desc.md
Outdated
* information about the deployed contract in the network indicated by the | ||
* key | ||
*/ | ||
[network: 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.
[network: string]: { | |
[network: 'mainnet' | 'testnet' | 'betanet' | 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.
Current one is base64 genesis hash, but I feel like we should support the more understandable short-hand?
Side-note: just realised this is part of ARC-0004. I've never seen anyone use this, it's a bit tricky if you are auto-generating the json file to include stateful data like this. We also don't currently support it in algokit utils, although if we want to support it we should add that! This would be handy when third parties like oracles are distributing a file.
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.
Part of the rationale for the base64 is that is allows one to accurately describe a private network. We could have a separate entry for public networks though, but because this is so seldomly used I'm not sure if it'd really be worth it.
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 suggestion above still has string
so the implementation would be it's either base64 or one of the special known network names. It would make it a lot easier to use this feature.
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 is minor though happy if it's left out.
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 that works with me. I'll just specify in the ARC that the human names are NOT backwards compatible and MUST be accompanied by the genesis hash as well in the object with the same app IDs.
ARCs/arc-draft_extended_desc.md
Outdated
*/ | ||
[network: string]: { | ||
/** The app ID of the deployed contract in this network */ | ||
appID: number; |
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.
Worth adding in the AlgoKit format as well as another option? i.e. creator address + name
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'm trying to think how this could be done in a way that is ARC4 backwards compatible. Don't want to omit appID because that could break an ARC4 client if it tries to use that field (no clients in existence use it that I know of, but just want to be safe)
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 nothing actually uses this right now then I think it's OK to mess with it and note is as a known deviation from ARC4?
Again, pretty minor feature though.
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 possible, since we tried to keep ARC4 backward compatibility, I would not pursue this minor deviation. I think declaring the association between appID
and the Network ID on which the App is deployed has some value.
ARCs/arc-draft_extended_desc.md
Outdated
readonly: boolean; | ||
} | ||
|
||
interface Contract { |
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.
Given this is the driving interface suggest this is added at the top of the code snippet, possibly also worth having a comment to indicate this is an extension of ARC4?
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.
Added comments in a18a968. I used this ordering because it ensures that any interface is described before it's actually used, which makes more sense to me but happy to change it if others feel otherwise.
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's a bit hard to comprehend right now. I'd advocate for splitting it out into multiple definitions and indicate with narrative that the definitions are further down the page.
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.
Let me know what you think of the recent update
ARCs/arc-draft_extended_desc.md
Outdated
/** The type of the key. Can be ABI type or named struct */ | ||
keyType: string; | ||
/** The type of the value. Can be ABI type or named struct */ | ||
valueType: 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.
Wonder if this should be StructName | ABIType
or similar?
Also, previously we had avm type, do we need to support that too / instead? Having the proper type would be awesome because it's an annoying limitation of the current stuff.
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.
Wonder if this should be StructName | ABIType or similar?
Yeah I like that. Will make the change.
Also, previously we had avm type, do we need to support that too / instead? Having the proper type would be awesome because it's an annoying limitation of the current stuff.
Initially I was thinking it wasn't necessary because it's obvious when one is using uint64 vs ABI uint64, but we do need a way to specifcy AVM bytes (no prefix) vs ABI bytes. i'll add AVMType as well.
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 added AVMBytes
type in a18a968. I don't think uint64 is needed because it's always going to be obvious when AVM uint64 is used (only top-level values in global/local)
ARCs/arc-draft_extended_desc.md
Outdated
}; | ||
}; | ||
/** Describes single key-value pairs in the application's state */ | ||
keys: { |
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.
Previous one had reserved and declared values, I've honestly never used reserved, but I guess it makes sense where you want to have an upgradeable contract that preallocates more storage than it currently needs?
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.
Same re: static
that was there previously, do we need it?
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 didn't make a differentiation between reserved and declared because schema is immutable, thus it would make sense to always just use the max amount of state when creating the app (declared + reserved).
Same re: static that was there previously, do we need it?
I don't see static in ARC32 itself. How is it 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.
Yeah I don't think static is used so I'm happy to omit
I wonder if a better way then is to allow for you to override the number of ints/bytes rather than having reserved (which I always thought was weird).
ARCs/arc-draft_extended_desc.md
Outdated
## Specification | ||
```ts | ||
/** Mapping of named structs to the ABI type of their fields */ | ||
interface StructFields { |
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 one is an associative array, but all of the other things seem to be arrays with objects that have a name property. Should we be consistent?
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'm not sure I follow, what do you mean?
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.e. this one is:
{
fieldName: ...
}
rather than the others parts of the spec that are:
[{name: "fieldName", ...}]
I know the eventual goal is to use simulate, but I wonder if we add in optional per-method hints for things like:
And also:
Are there other things that have come up in the past worth considering while we are looking at this? |
My rationale for not including a field for these sort of things is that they can be dynamic which can be hard to describe in JSON and they could be network dependent. It also is not easy for a HLL to autogenerate these sorts of things and if the dev is required to manually write them out might as well just have them write a standardized readonly method that returns this information so they have the full power of the language and contract state/methods at their disposal (see ARC51).
Yes good call. |
Co-authored-by: Rob Moore (MakerX) <robertmooreweb@gmail.com>
@SudoWeezy I think we are ready to merge and move to last call |
/** Description of what this storage key holds */ | ||
desc?: string; | ||
/** The type of the key */ | ||
keyType: ABIType | AVMBytes | StructName; |
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
? Maybe keyEncoding
would be clearer? I don't think it benefits from consistency of naming with StorageMap.keyType
since that seems of a different purpose - to indicate what sort of value to encode to combine with StorageMap.key
in order to retrieve a value. Whereas StorageKey.key
is already encoded and you may want to know how to interpret that value. So they're kind of opposites?
This is a much-needed ARC and everything looks great. When merge? |
Very close. Initial generation and consumption of ARC56 has been spiked out and things look good so far. Unless something is caught during Puya integration I think this is ready to be finalized |
/** The type of the values in the map */ | ||
valueType: ABIType | AVMBytes | StructName; | ||
/** The prefix of the map, encoded as a utf-8 string */ | ||
prefix?: 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.
Why utf-8
here and not base64
like StorageKey.key
? UTF-8 is more readable, but does that not preclude raw byte prefixes?
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.
Good point. I can change to base64
/** The pre-compiled TEAL that may contain template variables. MUST be omitted if included as part of ARC23, but otherwise MUST be defined. */ | ||
source?: { | ||
/** The approval program */ | ||
approval: 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.
can we make this be the TEAL or the bytecode?
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.
Are you saying have two properties? One for pre-compiled TEAL and one for compiled bytecode?
ARCs/arc-0056.md
Outdated
}; | ||
/** ARC-28 events that MAY be emitted by this contract */ | ||
events?: Array<Event>; | ||
/** A mapping of template variable names as they appear in the teal (not including TMPL_ prefix) and their respecive types */ |
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 named byte
which subsequently overwrites all byte
opcodes which is not the intended effect of template variables.
/** The arguments of the event, in order */ | ||
args: Array<{ | ||
/** The type of the argument */ | ||
type: ABIType; |
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.
Since this isn't part of ARC-4, maybe in the case of a struct, we can avoid re-specifying it as a tuple here?
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 is not a part of ARC4 but it is a part of ARC28. You could make the argument that ARC28 doesn't have much adoption yet, but I think it's better to keep backwards compatibility with existing ARCs
So there's potentially three different kinds of stages that we can think about here:
In scenario one, clearly the TEAL is needed, as well as the template variables available. It's not possible to have In scenario two, there is In scenario three, the network -> appID mapping can be populated. Bytecode is perhaps optional, but sourceInfo should be retained (but optional). Some other things worth mentioning: |
I've also realized in this scenario we should include the template variables used as well so the source map can be reconstructed. |
Makes sense (as an optional thing). It may be that you want to distribute an app spec for consumers without source code so general public can consume it so all of the source stuff needs to be optional. |
I don't feel strongly but one reason to enforce it to exist is to preserve error messages |
You could include PC => error message only though in that case? |
One thing that I also want to add that we've missed: scratch slots. |
Just did the following
|
Basically takes most of the ideas in ARC32 and reorgs it in a way that is backwards compatible with ARC4 clients. Main benefit of this is HLLs can simply generate one JSON artifact.