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

Transaction role prepare blocks take one signer each #2284

Conversation

turbolent
Copy link
Member

@turbolent turbolent commented Jan 31, 2023

Work towards #2177

Description

As clarified in the FLIP, a prepare block in a transaction role may have one parameter, and each role is passed one signer, sequentially from the full list of all signers.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@turbolent turbolent requested a review from a team January 31, 2023 18:02
@turbolent turbolent self-assigned this Jan 31, 2023
Base automatically changed from bastian/extended-transaction-format-5 to feature/extended-transaction-format January 31, 2023 18:04
@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Merging #2284 (04c2a30) into feature/extended-transaction-format (f6f0fc3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                         Coverage Diff                          @@
##           feature/extended-transaction-format    #2284   +/-   ##
====================================================================
  Coverage                                77.81%   77.81%           
====================================================================
  Files                                      309      309           
  Lines                                    66207    66230   +23     
====================================================================
+ Hits                                     51517    51540   +23     
  Misses                                   12906    12906           
  Partials                                  1784     1784           
Flag Coverage Δ
unittests 77.81% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/interpreter/interpreter_transaction.go 98.87% <100.00%> (+0.01%) ⬆️
runtime/sema/check_transaction_declaration.go 96.19% <100.00%> (+0.17%) ⬆️
runtime/sema/errors.go 94.98% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

runtime/sema/check_transaction_declaration.go Outdated Show resolved Hide resolved
Comment on lines +608 to +616
prepare(first: AuthAccount, second: AuthAccount) {}

role role1 {
prepare(signer: AuthAccount) {}
}

role role2 {
prepare(signer: AuthAccount) {}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this feature would benefit from some way to encode in Cadence which of the signers in the outer prepare is tied to which role; but that may be beyond the scope of this PR and might just be another discussion point for the FLIP.

Co-authored-by: Daniel Sainati <sainatidaniel@gmail.com>
@github-actions
Copy link

Cadence Benchstat comparison

This branch with compared with the base branch onflow:feature/extended-transaction-format commit f6f0fc3
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -benchmem -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Collapsed results for better readability

@turbolent turbolent requested a review from a team February 1, 2023 18:54
@turbolent
Copy link
Member Author

@SupunS Could you please have a look? It's (currently) the last PR for the feature, and would allow us to get a preview build out. Thanks!

if roleCount > 0 &&
prepareFunctionDeclaration != nil &&
transactionPrepareParameterCount > 0 &&
roleCount != transactionPrepareParameterCount {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do they have to be equal? roleCount <= transactionPrepareParameterCount should be also fine?
i.e: There can be accounts used by the transaction's prepare only, but not by any roles.

But maybe this should be also something to discuss on the FLIP

@turbolent turbolent merged commit 42c3010 into feature/extended-transaction-format Feb 2, 2023
@turbolent turbolent deleted the origin/bastian/extended-transaction-format-6 branch February 2, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants