-
Notifications
You must be signed in to change notification settings - Fork 35
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
Assert that Atomic Transaction Composer's method adder checks for arg-length consistency #189
Conversation
…h between method signature params and runtime args
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.
On the whole looks good, but I have some suggestions:
- The new scenario can be completely tested without a network, so I think it would be better off as a unit test in
features/unit/atomic_transaction_composer.feature
. This will save time, since the lengthyBackground
section for integration tests does not need to run again for another scenario. - Using a regex to evaluate error messages across SDKs has shortcomings, since each SDK tends to word their error messages slightly different. Instead, I'd recommend using the same approach as the step
I build the transaction group with the composer. If there is an error it is "<error>"
-- if you look at the implementations for this method (e.g., the go SDK's), the<error>
string is not taken literally, it's more like an enum value that tells the SDK what class of error should have happened, and lets the SDK validate its own actual error message.
Thanks for approving @jasonpaulos. I cannot merge as is since it would fail the SDK tests. @michaeldiamant what do you think. Should I add a new tag |
@tzaffi I'd favor a new tag. Proliferating tags is a concern, but longstanding. open PRs feels to me like a worse tradeoff. |
Method Adder arg-length Check Assertion
Adding a new integration test Scenario Outline that asserts that when adding an ABI method via the Atomic Transaction Composer, a validity check is undertaken to ensure that the number of runtime args is the number that is expected by the method's signature. In particular, a new step is introduced, which is a variant of the "I add a method call with..." step family.
Will not merge until all 4 SDK's implement the new step, or will at least add a new tag before merging.
SDK Specific Situations
Please see #190 for some cursory investigations that were conducted on how each SDK's
AtomicTransactionComposer
asserts that the number of arguments provided to the app call, match the number according the method signature.