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.

Sign up for GitHub

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

Jump to bottom

WIP: tx generate only #3287

Closed
wants to merge 14 commits into from
Closed

WIP: tx generate only #3287

wants to merge 14 commits into from

Conversation

faboweb
Copy link
Contributor

@faboweb faboweb commented Jan 12, 2019 •
edited by alexanderbez
Loading

closes: #3284

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@faboweb faboweb changed the title [WIP] Fabo/generate only Fabo/3284 tx generate only Jan 14, 2019
@jackzampolin
Copy link
Member

Looks like it's still failing tests...

--- FAIL: TestCoinSendGenerateSignAndBroadcast (2.20s)
	Error Trace:	lcd_test.go:246
	Error:      	Not equal: 
	            	expected: 200
	            	actual  : 400
	Test:       	TestCoinSendGenerateSignAndBroadcast
	Messages:   	decoding Bech32 address failed: must provide an address

fedekunze
fedekunze reviewed Jan 14, 2019
View reviewed changes
client/utils/rest.go Outdated
accNumber, seq uint64, fees sdk.Coins, genOnly, simulate bool) BaseReq {

return BaseReq{
Name: strings.TrimSpace(name),
Password: password,
From: strings.TrimSpace(from),
Copy link
Collaborator

Choose a reason for hiding this comment

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

from should be an sdk.AccAddress to avoid checking if the string is a valid address

@codecov
Copy link

codecov bot commented Jan 14, 2019

Codecov Report

Merging #3287 into develop will decrease coverage by 0.02%.
The diff coverage is 86.66%.

@@             Coverage Diff             @@
##           develop    #3287      +/-   ##
===========================================
- Coverage    55.18%   55.16%   -0.03%     
===========================================
  Files          134      134              
  Lines         9526     9517       -9     
===========================================
- Hits          5257     5250       -7     
+ Misses        3938     3936       -2     
  Partials       331      331

fedekunze
fedekunze previously approved these changes Jan 15, 2019
View reviewed changes
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK

jackzampolin
jackzampolin approved these changes Jan 15, 2019
View reviewed changes
Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

Changes look good @faboweb and @fedekunze

jackzampolin
jackzampolin requested changes Jan 15, 2019
View reviewed changes
x/bank/client/rest/sendtx.go
utils.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
return
var from sdk.AccAddress
if req.BaseReq.GenerateOnly {
Copy link
Member

Choose a reason for hiding this comment

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

So if I'm reading this right, on generate only it just adds a From address to the base req and then will CompleteAndBroadcastTxREST right? If so this would need some more modification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It takes the from address from base_req instead of resolving it from the username.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plus it doesn't complete and broadcast it yes. Probably needs more modification. Can someone take this over who is opinionated about the SDK code?

@fedekunze fedekunze changed the title Fabo/3284 tx generate only WIP: tx generate only Jan 16, 2019
@fedekunze fedekunze added the wip label Jan 16, 2019
@fedekunze fedekunze dismissed their stale review January 16, 2019 14:20

need to address changes

@fedekunze fedekunze assigned fedekunze and unassigned fedekunze Jan 16, 2019
@fedekunze
Copy link
Collaborator

@alessio or @alexanderbez can you work on this if any of you have time ?

@alexanderbez
Copy link
Contributor

I'll pick it up sure 👍

@alexanderbez alexanderbez self-assigned this Jan 18, 2019
@faboweb
Copy link
Contributor Author

faboweb commented Jan 18, 2019

Thx @alexanderbez, would have done it myself but I think you know better how you want this to be designed. Happy to help if you need any.

@alexanderbez
Copy link
Contributor

@faboweb closing this in favor of #3396

@cwgoes cwgoes deleted the fabo/generate_only branch January 25, 2019 16:29
@snyk-bot snyk-bot mentioned this pull request Dec 23, 2020
Open
@Itchibon777 Itchibon777 mentioned this pull request Jan 1, 2024
Open
@Itchibon777 Itchibon777 mentioned this pull request Mar 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackzampolin jackzampolin jackzampolin requested changes

@fedekunze fedekunze fedekunze left review comments

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

Assignees

@alexanderbez alexanderbez

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Generate_only should create txs without requiring a local key nor password
4 participants
@faboweb @jackzampolin @fedekunze @alexanderbez