Skip to content

Add prompts for build and send-signed#432

Merged
Kay-Zee merged 2 commits into
masterfrom
kan/transaction-confirmation-screen
Jan 12, 2022
Merged

Add prompts for build and send-signed#432
Kay-Zee merged 2 commits into
masterfrom
kan/transaction-confirmation-screen

Conversation

@Kay-Zee
Copy link
Copy Markdown
Member

@Kay-Zee Kay-Zee commented Dec 22, 2021

Closes #410

Description

Adds prompt for the sendsigned and build commands.

Currently auto accepts for the send command, as this is currently used in ways that make upgrading it less convenient and would require other work in additional repos. It's also less important, as it's a single step process so it should be "easier" to know what is being sent. Still would be good to have, but i think is a bigger lift for a smaller gain, so better to just tackle to two higher value commands first.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #432 (3b5d7c0) into master (22bb301) will decrease coverage by 0.12%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #432      +/-   ##
==========================================
- Coverage   55.50%   55.38%   -0.13%     
==========================================
  Files          36       36              
  Lines        1906     1914       +8     
==========================================
+ Hits         1058     1060       +2     
- Misses        711      716       +5     
- Partials      137      138       +1     
Flag Coverage Δ
unittests 55.38% <33.33%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/flowkit/services/transactions.go 60.83% <33.33%> (-2.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22bb301...3b5d7c0. Read the comment docs.

Comment thread pkg/flowkit/services/transactions.go Outdated
Comment on lines +198 to +199
if !approveSend {
if !output.ApproveTransactionForSendingPrompt(tx) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
if !approveSend {
if !output.ApproveTransactionForSendingPrompt(tx) {
if !approveSend && !output.ApproveTransactionForSendingPrompt(tx) {

@devbugging
Copy link
Copy Markdown
Contributor

Thank you this is great. We just need to be careful to update LS accordingly as it uses this API. I can make a PR on top of that.

The lining error is a nasty one, I was doing an update of dependencies and I will need to make a PR on Go SDK to stop using deprecated functionality, so as long as this is the only listing error I think we can merge until I fix that with PR, I plan to do that as soon as I get the time.

@devbugging devbugging self-requested a review January 6, 2022 13:06
Copy link
Copy Markdown
Contributor

@devbugging devbugging left a comment

Choose a reason for hiding this comment

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

🚀

@Kay-Zee Kay-Zee merged commit 0dbd394 into master Jan 12, 2022
@devbugging devbugging deleted the kan/transaction-confirmation-screen branch January 13, 2022 10:09
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.

Transaction confirmation screen

3 participants