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(cmd): Add support for outputting unsigned transactions #67

Merged
merged 3 commits into from May 15, 2023

Conversation

kostko
Copy link
Member

@kostko kostko commented May 9, 2023

No description provided.

@kostko kostko requested a review from matevz May 10, 2023 06:23
@kostko kostko force-pushed the kostko/feature/unsigned-tx-output branch from 107c39a to 0b9bbdd Compare May 10, 2023 06:24
cmd/common/transaction.go Show resolved Hide resolved
cmd/common/transaction.go Outdated Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/unsigned-tx-output branch from 0b9bbdd to 0f01c74 Compare May 11, 2023 15:54
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Thanks for the code cleanup. I tested this and usage seems awkward. Example:

oasis account transfer 1.0 test:bob

will sign and broadcast the transaction.

One would expect that using the new -o switch would dump the tx to a file instead of broadcasting it:

oasis account transfer 1.0 test:bob -o transfer-bob.json

But it still broadcasts the transaction (and -o switch does nothing).

So what you do is put an --offline flag, but then you get

$ oasis account transfer 1.0 test:bob --offline -o transfer-bob.json
Error: nonce and/or gas limit must be specified in offline mode

I suggest the following: -o switch should mean "output tx to file instead of broadcasting it". But this should be separate from the --offline switch which should mean "don't use network to determine transaction parameters".

@kostko kostko force-pushed the kostko/feature/unsigned-tx-output branch from 0f01c74 to d42912f Compare May 12, 2023 10:52
@kostko
Copy link
Member Author

kostko commented May 12, 2023

Fixed, please take another look.

Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

I was trying to generate an unsigned transactio and then sign and submit it. Are two authorized signers expected here?

$ oasis account transfer 2.5 test:bob -o test.tx  --unsigned
Unlock your account.
? Passphrase: 
oa@matevz-oa:~/cli$ oasis tx submit test.tx 
Unlock your account.
? Passphrase: 
You are about to sign the following transaction:
Format: plain
Method: accounts.Transfer
Body:
  To: test:bob (oasis1qrydpazemvuwtnp3efm7vmfvg3tde044qg6cxwzx)
  Amount: 2.5 TEST
Authorized signer(s):
  1. MJ2XCjkj132C9YWpDUSQFjkCTI8bSw8bi0w9EwwE1Bg= (ed25519)
     Nonce: 3
  2. MJ2XCjkj132C9YWpDUSQFjkCTI8bSw8bi0w9EwwE1Bg= (ed25519)
     Nonce: 3
Fee:
  Amount: 0.001152295 TEST
  Gas limit: 230459
  (gas price: 0.000000005 TEST per gas unit)

Network:  testnet
ParaTime: cipher
Account:  test
? Sign this transaction? (y/N)

@kostko kostko force-pushed the kostko/feature/unsigned-tx-output branch from d42912f to 0d1037f Compare May 15, 2023 09:46
@kostko kostko force-pushed the kostko/feature/unsigned-tx-output branch from 0d1037f to 2952d3e Compare May 15, 2023 09:47
@kostko kostko force-pushed the kostko/feature/unsigned-tx-output branch from 2952d3e to 34dfef3 Compare May 15, 2023 09:51
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Good job!

@kostko kostko merged commit 72d2756 into master May 15, 2023
2 checks passed
@kostko kostko deleted the kostko/feature/unsigned-tx-output branch May 15, 2023 10:18
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.

None yet

2 participants