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

Fabo/1809 update header on send #1830

Merged
merged 132 commits into from
Jan 24, 2019
Merged

Fabo/1809 update header on send #1830

merged 132 commits into from
Jan 24, 2019

Conversation

faboweb
Copy link
Collaborator

@faboweb faboweb commented Jan 15, 2019

Closes #1809

Based on Web PR

Description:

❤️ Thank you!


  • Added entries in CHANGELOG.md with issue # and GitHub username
  • Reviewed Files changed in the github PR explorer

@faboweb
Copy link
Collaborator Author

faboweb commented Jan 18, 2019

Proposal submission works for me. But:

  • Updating the header also doesn't work properly on send.
  • Delegation gives me a signature verification failure.

const depositDenom = governanceParameters.deposit.min_deposit[0].denom

// TODO: refactor according to new unit test standard
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

@sabau
Copy link
Contributor

sabau commented Jan 18, 2019

Wow you were quicker than me to self-report those two 👍I left other minor stuff for the tests but once those problems on Send and Delegation are fixed I think we should merge. The object refactor may also be a separate PR right?

@faboweb faboweb changed the title Fabo/1809 update header on send [WIP] Fabo/1809 update header on send Jan 20, 2019
@faboweb faboweb added the blocked ✋ issues blocked by other implementations/issues label Jan 20, 2019
@faboweb faboweb changed the title [WIP] Fabo/1809 update header on send Fabo/1809 update header on send Jan 21, 2019
@faboweb faboweb removed the blocked ✋ issues blocked by other implementations/issues label Jan 21, 2019
Signed-off-by: Karoly Albert Szabo <[email protected]>
Copy link
Contributor

@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

Copy link
Contributor

@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.

[ERROR] Error fetching proposals: Unexpected token u in JSON at position 0

Not sure if it's related to this PR

Copy link
Contributor

@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.

manual test: there's an aprox 5 sec (1 block) delay when submitting proposals , gov modules need to be included on this PR

@faboweb
Copy link
Collaborator Author

faboweb commented Jan 23, 2019

Let's fix optimistic updates of proposal creation in another PR.

address() {
return this.user.address
},
unbondedAtoms() {
return this.num.shortNumber(this.user.atoms)
return this.num.shortNumber(this.liquidAtoms)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice! 💧

state.balances.push({ denom: `coin`, amount: `42` })

// add new
const balance = { denom: `leetcoin`, amount: `1337` }
Copy link
Collaborator

Choose a reason for hiding this comment

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

QUE PASA leetcoin

@jbibla jbibla merged commit a8bedb1 into web Jan 24, 2019
@faboweb faboweb deleted the fabo/1809-update-header-on-send branch January 24, 2019 09:37
@@ -1085,7 +1085,7 @@ Msg Traces:
return results
}

let tally_result = {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should have stayed the same since the final_tally_result update was only on the proposals not on the tally endpoint cc: @faboweb

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.

4 participants