Feature: Batch voting #62
Closed
fsmanuel wants to merge 3 commits from
feature/batch-voting into master
pull from: feature/batch-voting
merge into: :master
:master
:feature/event_data
:feature/use_ember_auto_import_debugging
:build-production
:feature/charts
:feature/batch-voting-2
:feature/infura-fallback-provider
:feature/20-contributions_from_events
:kredits-package
Labels
Clear labels
bug
dev environment
docs
duplicate
enhancement
feature
idea
invalid
kredits-1
kredits-2
kredits-3
question
security
ui/ux
wontfix
Something is not working
Config, builds, CI, deployment, etc.
Documentation
This issue or pull request already exists
Improving existing functionality
New functionality
Something to consider
Not a bug
Small contribution
Medium contribution
Large contribution
Looking for an answer
release
major
release
minor
release
patch
All your base are belong to us
User interface, process design, etc.
This won't be fixed
No labels
Milestone
No items
No Milestone
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: kredits/kredits-web#62
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
This is the counterpart to https://github.com/67P/truffle-kredits/pull/26
I can not test it at the moment because the estimated gas doesn't work.
Everything else should be done until we have a better design.
@bumi can you test with this branch. My internet is shitty... and I forgot what to do if the gas estimation fails.
will check it out (hopefully later today/evening).
if the gas estimate fails it either means it can not estimate the gas ;) ... because the contract has an error or I don't know.
That loop in the batch vote function for sure could be an issue...
Good. That sounds like it's your problem not mine 😃
😱
Shouldn't there be a way to suggest a gas amount to the wallet for a transaction? It would seem that this is a basic need for any complex contract interaction, as wallets cannot estimate everything on their own.
yes, but the wallet should be able to estimate the gas - "simulating" the execution should be a better estimate than some value that we provide.
Didn't you just have the issue that the wallet estimated wrong, by a factor of 10 or more? At least Metamask seems to be planning some improvements: https://github.com/MetaMask/metamask-extension/issues/3914
But my question was rather aimed at solving the problem for us right now, without relying on wallet implementations, so we can start using the system.
our issue was related to ganache (+ ethers maye...)
I first need to review the contract code most of the time if the estimation does not work something is wrong with the contract
actually it works well for me! pretty cool actually!
I did not have to change any gas settings... maybe @skddc can you also check it?
One issue:
I have 4 open proposals. I select two, vote for them. These get executed and get removed from the list of open proposals. BUT they stay selected. once I select the remaining proposals and do submit selected it also tries to submit the already submitted ones.
Good catch! I’ll remove the selection after the submit!
👍
we should remove executed proposals also. As maybe...maybe a proposal gets executed before I vote and then my whole batch vote fails.
It might also make sense to change the behaviour of the vote function and not fail if a proposal is already executed... but just count the vote but don't execute it?
Just as an aside, I wouldn't put much work in the UI there right now, as it's going to change entirely. There won't be any selection of contribution entries for the batch voting for example. As long as the underlying batch vote works for now, it should be good enough, and we can then get to the edge cases when we have the new UI.
we should still merge this one, right? it implements everything needed on batch voting independently of the exact UI.
the removal of the selection is missing then we're good to merge.
I'll check it out when I have everything running on Kovan on my machine.
I'll update in a sec.
Updated
@bumi @skddc can you have another look?
Yup, will do this next.
That comment should be removed
I get the following error:
I rebased this on master locally, but that shouldn't change anything I would assume.
@skddc that's probably because the ABI of the operator was not up to date: https://github.com/67P/kredits-contracts/commit/7216522d833f11e44377df86b1fde21fecae1e7e
I released a new version of kredits-contracts
3.0.2- can you try with that one?Pull request closed