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

goal: Allow --signer to send txns from rekeyed accounts #4175

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

jannotti
Copy link
Contributor

@jannotti jannotti commented Jun 24, 2022

We do, after all, live in a society.

Sprinkled some -S tests around in the e2e tests.

@jannotti jannotti changed the title Allow --signer to send txns from rekeyed accounts goal: Allow --signer to send txns from rekeyed accounts Jun 24, 2022
@jannotti jannotti self-assigned this Jun 24, 2022
@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Merging #4175 (ba4459f) into master (7bc8491) will decrease coverage by 0.05%.
The diff coverage is 12.00%.

@@            Coverage Diff             @@
##           master    #4175      +/-   ##
==========================================
- Coverage   54.62%   54.57%   -0.06%     
==========================================
  Files         391      391              
  Lines       48770    48788      +18     
==========================================
- Hits        26643    26628      -15     
- Misses      19903    19930      +27     
- Partials     2224     2230       +6     
Impacted Files Coverage Δ
cmd/goal/application.go 11.16% <0.00%> (ø)
cmd/goal/asset.go 16.25% <0.00%> (ø)
cmd/goal/clerk.go 8.76% <0.00%> (-0.17%) ⬇️
cmd/goal/account.go 12.62% <21.05%> (+0.19%) ⬆️
cmd/goal/interact.go 3.62% <50.00%> (+0.26%) ⬆️
cmd/goal/common.go 100.00% <100.00%> (ø)
cmd/algoh/blockWatcher.go 77.77% <0.00%> (-3.18%) ⬇️
ledger/blockqueue.go 82.18% <0.00%> (-2.88%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
... and 5 more

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 7bc8491...ba4459f. Read the comment docs.

We do, after all, live in a society.
@jannotti jannotti marked this pull request as ready for review June 27, 2022 15:31
@jannotti jannotti requested review from winder and barnjamin June 27, 2022 18:17
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Approving since this is objectively better than it was. If you're motivated, I think it go a little further...

If you actually swapped out all of the SignTransactionWithWallet calls, could we also get rid of SignTransactionWithWallet and/or make it private?

@jannotti
Copy link
Contributor Author

If you actually swapped out all of the SignTransactionWithWallet calls, could we also get rid of SignTransactionWithWallet and/or make it private?

I wanted to do that, but the use of createSIgnedTransaction from clerk.go was just annoying enough that I didn't want to sort it out. I think the basic trouble is that the ...AndSigner() call wants a text value for the signer address, while createSignedTransaction takes a basic.Address, so it needs SignTransactionWithWallet() unless I do some silly conversions back and forth. It's tempting. If I find myself in there again, I will.

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.

3 participants