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

Assert that Atomic Transaction Composer's method adder checks for arg-length consistency #189

Merged
merged 9 commits into from
Jun 17, 2022

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Jun 14, 2022

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.

I add a method call with the signing account, the current application, suggested params, on complete "noop", current transaction signer, current method arguments; any resulting exception has key "{none-or-exception-key}".

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.

…h between method signature params and runtime args
Copy link
Contributor

@jasonpaulos jasonpaulos left a 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 lengthy Background 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.

@tzaffi tzaffi requested a review from jasonpaulos June 16, 2022 19:27
@tzaffi
Copy link
Contributor Author

tzaffi commented Jun 17, 2022

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 @unit.atcMethodArgs (or something like that) and then merge? Or should I leave it open till all the SDK's have implemented?

@michaeldiamant
Copy link
Contributor

Should I add a new tag @unit.atcMethodArgs (or something like that) and then merge? Or should I leave it open till all the SDK's have implemented?

@tzaffi I'd favor a new tag. Proliferating tags is a concern, but longstanding. open PRs feels to me like a worse tradeoff.

@tzaffi tzaffi merged commit 5ddc413 into master Jun 17, 2022
@tzaffi tzaffi deleted the abi-too-many-args branch June 17, 2022 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants