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

feat: Add --argument-file option to dfx canister sign #2787

Merged
merged 10 commits into from Dec 1, 2022

Conversation

Maximkaaa
Copy link
Contributor

Description

dfx canister call command has an argument --argument-file to allow sending the request when argument string is too long for shell. dfx canister sign command didn't have this argument so some of the requests possible by dfx canister call were not possible to be executed through sign/send workflow. This PR fixes this unfortunate inconvenience:

  • add --argument-file option to sign command
  • move the logic related to reading argument string from a file from call command to utils so that it can be reused by both call and sign
  • refactor the logic a little to use correct error handling instead of expects

How Has This Been Tested?

Added e2e test cases. Also did signing manually with compiled dfx, including some very long argument strings that didn't fit into CLI arguments.

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@Maximkaaa Maximkaaa requested a review from a team as a code owner November 29, 2022 19:49
@dfinity-droid-prod
Copy link

Dear @Maximkaaa,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA1.

If you decide to agree with it, please visit this issue and read the instructions there.

— The DFINITY Foundation

Footnotes

  1. Contributor License Agreement

@dfinity-droid-prod
Copy link

Dear @Maximkaaa,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA1.

If you decide to agree with it, please visit this issue and read the instructions there.

— The DFINITY Foundation

Footnotes

  1. Contributor License Agreement

Copy link
Contributor

@sesi200 sesi200 left a comment

Choose a reason for hiding this comment

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

Thank you very much for such a complete PR! Even changes to the docs are included!

I have a few small suggestions for the tests, otherwise this looks great.

docs/cli-reference/dfx-canister.md Outdated Show resolved Hide resolved
e2e/tests-dfx/sign_send.bash Outdated Show resolved Hide resolved
e2e/tests-dfx/sign_send.bash Outdated Show resolved Hide resolved
e2e/tests-dfx/sign_send.bash Show resolved Hide resolved
e2e/tests-dfx/sign_send.bash Show resolved Hide resolved
Co-authored-by: Severin Siffert <severin.siffert@dfinity.org>
@Maximkaaa
Copy link
Contributor Author

Thanks for the quick review and suggestions. Yes, I also wanted to check the sign result in the tests but was not sure how to go about it.

@sesi200
Copy link
Contributor

sesi200 commented Nov 30, 2022

I'm not 100% sure this works either, but I think it should. Would you mind going through the CLA bot process? Otherwise I'm not allowed to merge this. (Also, can you do it for both issues? It has a bug where it duplicates everything and if you just go through it for both issues it opened it's quickest)

Copy link
Contributor

@sesi200 sesi200 left a comment

Choose a reason for hiding this comment

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

This should fix the sign_send test failures

e2e/tests-dfx/sign_send.bash Outdated Show resolved Hide resolved
e2e/tests-dfx/sign_send.bash Outdated Show resolved Hide resolved
Co-authored-by: Severin Siffert <severin.siffert@dfinity.org>
@Maximkaaa
Copy link
Contributor Author

@sesi200 I applied the suggested change and completed cla process in both issues.

@sesi200
Copy link
Contributor

sesi200 commented Nov 30, 2022

Thanks a lot, @Maximkaaa. I'll only have to get #2788 merged, and then this is good to merge, too

e2e/tests-dfx/sign_send.bash Outdated Show resolved Hide resolved
e2e/tests-dfx/sign_send.bash Outdated Show resolved Hide resolved
e2e/tests-dfx/sign_send.bash Outdated Show resolved Hide resolved
Maximkaaa and others added 3 commits November 30, 2022 16:12
Co-authored-by: Severin Siffert <severin.siffert@dfinity.org>
Co-authored-by: Severin Siffert <severin.siffert@dfinity.org>
@Maximkaaa
Copy link
Contributor Author

I've fixed the last e2e test. The difference between expected and actual was due to the ordering of fields in the json, actual message is same.

@sesi200
Copy link
Contributor

sesi200 commented Nov 30, 2022

👍 then all that is left is to merge master

CHANGELOG.md Outdated Show resolved Hide resolved
@Maximkaaa
Copy link
Contributor Author

It seems that the order of fields in the response is not fixed so the tests flicker. So I changed the approach to checking the signed values. Instead of sending and checking the response, why not just check the arguments in the signed message file?

@sesi200 sesi200 merged commit 23c53f1 into dfinity:master Dec 1, 2022
@sesi200
Copy link
Contributor

sesi200 commented Dec 1, 2022

That works as well. Thanks!

@Maximkaaa Maximkaaa deleted the sign-argument-file branch December 1, 2022 08:14
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