-
Notifications
You must be signed in to change notification settings - Fork 141
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
Interpret transaction role declarations #2259
Interpret transaction role declarations #2259
Conversation
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.
Would be nice to see some tests for the multi-signer use case discussed in the FLIP, where each signer is a different role.
Added tests for multiple roles and multiple signers |
@SupunS Could you please have a look? |
Codecov Report
@@ Coverage Diff @@
## feature/extended-transaction-format #2259 +/- ##
=======================================================================
+ Coverage 77.77% 77.79% +0.01%
=======================================================================
Files 309 309
Lines 66111 66164 +53
=======================================================================
+ Hits 51420 51474 +54
+ Misses 12907 12906 -1
Partials 1784 1784
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it 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.
👌
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/extended-transaction-format commit fa2c9fe Collapsed results for better readability
|
function only currently invokes prepare, but might do more in the future. e.g like transactions, role blocks may get support for conditions
@dsainati1 Could you please have a look? That would unblock getting the PR stack / this feature in. Thanks! |
Perhaps I am misunderstanding, but this part of the FLIP implied to me that we should support something like this, where roles would allow each signing account to know which values and variables they are responsible for:
Where each signer is solely responsible for a different role. It's not clear to me that we support this in this implementation. If we do can we add a test case for it? |
let signer1Address: String | ||
let signer2Address: String | ||
|
||
prepare(signer1: AuthAccount, signer2: AuthAccount) { |
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.
Should this be allowed? If each signer should have 1 role, we probably need to enforce that each role has one signer and also there are an equal number of roles to signers.
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.
Discussed with Bastian, this will be addressed in a followup PR. For now we can accept this and fix it later.
Work towards #2177
Description
Interpret transaction roles. Call each role's prepare block just like the transaction's prepare block before.
master
branchFiles changed
in the Github PR explorer