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
FLIP - Extended Transaction Format #41
base: main
Are you sure you want to change the base?
FLIP - Extended Transaction Format #41
Changes from 3 commits
78108e5
b5f5a06
aa7f626
6a57103
c2ff945
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.
Can all post-statements access variables with any role?
I assume the execute statement can access any variable regardless of the role? Can they mutate 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.
Cadence already has support for pragma declarations, which have the syntax
'#' expression
. They are not used for anything yet, but were added early on in anticipation of such features. A use like#role: Seller
is thus currently syntactically invalid. A valid use could be e.g.#role(Seller)
, which in terms of the AST would be expressed as a invocation expression of identifierrole
, with one argument, the identifierSeller
. Another valid use could be e.g.#tx(role: Seller)
.Also, pragma declarations are self-standing, they are not associated with any other declaration. However, for a transaction declaration, Cadence could look specifically for preceding pragma declarations and consider them.
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.
Another alternative here is to parse the docstring of said transaction, i have a very crude attempt at that in a branch in overflow. the transaction i process there is here https://github.com/bjartek/overflow/blob/flix2/transactions/mint_tokens.cdc
However i think pragma is the better approach here, especially if it can be syntax checked. Since a comment is well a comment.
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 sounds more like scoping to me. What if a new "role block" is introduced?
Something like:
So that it's clear on the scoping rules, and also:
I'm not quite sure what the variable access semantic should be though. One option is to make them seamlessly available to
execute
andpost
blocks. Another option would be to use role-qualified-names. e.g:seller.nft
, etc. The latter would allow you to have separate namespaces for roles (so don't have to worry about duplicate names)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.
Great idea! This scoped approach would also solve the problem of definite initialization in the
prepare
block 👍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 really like this idea! Having a new block would solve this and the related problems pretty neatly, and also be fairly clear to a reader.
I'm curious though whether
pre
andpost
blocks need to be similarly separated though; as with the upcoming changes in Stable Cadence to restrict condition blocks to beingview
, they won't be subject to the same concerns about malicious use of accounts and data thatprepare
functions are. Seems reasonable to have these conditions exist outside ofrole
blocks and make any variables defined within those blocks available outside them.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.
While implementing type checking for role blocks I realized the current resource semantics will not allow roles blocks to have resource-kinded fields, because they somehow need to be invalidated.
In the case of a transaction, the execute block acts as the destructor and allows invalidation of resource-kinded fields.
However, in role blocks, there is no execute block, but rather the outer transaction's execute block should probably be allowed to invalidate role blocks' resource kinded fields.
For example, we probably want to allow something like this example, which is currently rejected (correctly) due to the nested move in the last statement.
Trying to work around this by e.g. making the field optional, for which nested moves are allowed, still gets rejected (correctly), because the optional resource-kinded field could still have a value, and there is no destructor which invalidates it.
Support for resource-kinded fields in roles is probably wanted, for example one could imagine a
buyer
role which gets the FTVault
needed to purchase an item.If we do want to support resource-kinded fields in roles, should we extend resource invalidation and allow for nested field invalidation? For the first example above: the transaction's
buyer
field is indirectly resource-kinded, because it has a resource-kinded field, so must be invalidated – given that roles are not first-class values, at the role's resource-kinded fields must be invalidated. Currently this is limited to just fields ofself
.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.
What if instead of thinking of the
role
like an actual object in the language with fields of its own, we treated it like a namespace? This way we wouldn't have to special case its "fields", because those fields would exist on the outer transaction's scope, they would just be namespaced to their role.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 like this idea. This is exactly how I had imagined when I first proposed the syntax. Role-block is just a named-block/named-scope and nothing else. For eg., many languages support the block syntax
{ }
at the statement level to allow having a separate scope for certain things, and this is also that, but with a label.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.
How would this namespacing be implemented? I'm not saying it couldn't, I'm really just trying to think through this alternative and how it could be achieved, as well as how it would compare to other solutions.
For the definite initialization checking in the prepare blocks, the transaction's prepare block would need to ignore all of the roles' fields, and vice-versa, each role block would need to only consider its own fields and ignore all other fields?
For the resource checking, we would still need to adjust it and allow nested invalidation, because AFAICS currently it is implemented on a syntax level. For example, in the example above,
<-self.buyer.x
would internally just refer toself
's "buyer_x
" field, but syntactically it still looks like a nested access.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.
Luckily, allowing nested invalidation of resource-kinded fields of transaction roles turned out to be not hard to implement: onflow/cadence#2262. I think that resolves this discussion
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.
Do I understand it correctly, that the idea is that this empty template is presented to multiple wallets, and they each fill in this empty/partially filled transaction?
In that case we would need to expand the syntax to allow empty blocks.
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.
From prior discussions on this I belive this assumption is true.
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 will be ping pong little, proposer will propose, then it will go wallets they will add their prepares, it will go back to proposer, then proposer will merge them, then it will go to signers, each of them will sign and send back to proposer, then proposer will send to payer, payer will sign, then send back to proposer, proposer will send to network :)
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.
Supporting multiple prepare blocks, post conditions, etc. in the parser is no problem.
Given that prepare blocks currently checked like initializers, i.e. they must initialize all fields of the transaction, this check would need to be extended to support partial initialization in one block, and only consider all blocks together as the total set of initialized fields.
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.
How would a case of multiple prepare statements with varying prepare block parameter lists be handled?
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 am not OP here, but we have discussed this a bit in various meetings and I am quite passionate about this so i I will chime in. Feel free to correct me @JeffreyDoyle.
My understanding is that each prepare block can be filled out indepedently and composed into a transaction.
So lets say we have a buyer and a seller with two prepare blocks, for the seller we need to prepare the receiving vault while for the buyer we might prepare both the sending vault and the receiving collection/capability.
This also ties into flix where each flix can be a single role here if my understanding is correct. So the buyer pre/post pair would be one flix and the seller pre/post could be another.
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.
And that the seller has 20 flow less. @bluesign have propossed that mutating state should have mandatory post conditions. It is a lot of work but it is very 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.
What about royalty in this scenario? Should the transaction care about that aswell. In .find for instance royalty is handled by the contract not by the transaction, so that it cannot be skipped. In this case i guess the pre block for this role would fetch multiple variables for fund receivers and assert on all of them.