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

CLI signing improvements #2668

Merged
merged 9 commits into from
Sep 1, 2022
Merged

CLI signing improvements #2668

merged 9 commits into from
Sep 1, 2022

Conversation

roman-khimov
Copy link
Member

Problem

#2662, #2664

Solution

Solve 'em.

Here we're either saving or sending a transaction (depending on `out`), but
not both. Refs. #2664.
@roman-khimov roman-khimov added the cli Command line interface label Aug 31, 2022
@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #2668 (411ebdf) into master (673c895) will increase coverage by 0.02%.
The diff coverage is 89.21%.

@@            Coverage Diff             @@
##           master    #2668      +/-   ##
==========================================
+ Coverage   85.12%   85.14%   +0.02%     
==========================================
  Files         320      322       +2     
  Lines       39267    39398     +131     
==========================================
+ Hits        33426    33547     +121     
- Misses       4500     4515      +15     
+ Partials     1341     1336       -5     
Impacted Files Coverage Δ
pkg/rpcclient/nep11.go 0.00% <ø> (-2.86%) ⬇️
pkg/rpcclient/nep11/base.go 100.00% <ø> (ø)
pkg/rpcclient/nep17.go 18.18% <ø> (-2.60%) ⬇️
cli/paramcontext/context.go 61.29% <45.45%> (+1.29%) ⬆️
cli/util/send.go 78.57% <78.57%> (ø)
cli/wallet/nep17.go 74.82% <86.91%> (+1.15%) ⬆️
cli/wallet/multisig.go 80.76% <87.09%> (+2.82%) ⬆️
cli/wallet/wallet.go 85.18% <93.61%> (+0.45%) ⬆️
cli/wallet/nep11.go 85.48% <93.87%> (+5.19%) ⬆️
pkg/smartcontract/context/context.go 86.14% <95.83%> (+3.03%) ⬆️
... and 20 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Minor typo, otherwice LGTM.

And I have a question, currently, we can't provide arguments for deployed contract signature (if it requires parameters). Can't we extend existing signing interface to work with deployed contract with parameters? It would be nice to have it, maybe create a separate issue?

@roman-khimov
Copy link
Member Author

we can't provide arguments for deployed contract signature (if it requires parameters)

Deployed contracts have them in the manifest, so it should be used if needed. The bigger problem is custom contracts (I can have anything in my verification script) where parameters do need to be specified manually, there is no way around that. An issue, please (not a top priority, but can be done some day).

We can technically have more signatures in the file than we need and it's OK,
this case should be handled.
It's not needed, we already have the hash and getDecryptedAccount can't return
an account for a different one.
And document the behavior better. Fixes #2664.
It can be both NEP-11 and NEP-17.
To strip keys from wallets.
See documentation update for an example. Some code is made generic as well,
GetCompleteTransaction can now be used directly on ParameterContext.
@roman-khimov roman-khimov merged commit 20224cb into master Sep 1, 2022
@roman-khimov roman-khimov deleted the cli-signing-improvements branch September 1, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Command line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants