-
Notifications
You must be signed in to change notification settings - Fork 80
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
Cross-DAO Redemptions #1047
Cross-DAO Redemptions #1047
Conversation
I'll rebase this PR later today, but other than that, it's up for review. |
I've rebased it now. Be sure to have a close look at 404fe03. It's unrelated to the goal of this PR, but I noticed so much unused CSS classes that I thought it would help future development to clean that up a bit. |
@Procrat many thanks for the PR 💪 🔥 i've gone over the code very quickly, but it probably should be first reviewed by our product manager, @A-Zak . After he gives the ok for the functionality, I'll review in more detail There is a preview app on heroku here for this PR: https://alchemy-staging-rinkeb-pr-1047.herokuapp.com/ (talking to rinkeby) Also thanks for the cleanups and the CSS, much appreciated :-). Perhaps next time, if there will be one, this can be a separate PR? |
@jellegerbrandy Thanks for that. And yeah, I'll be sure to make a separate PR for separate things next time. Sorry about that. It sounded like @A-Zak would be busy with Berlin stuff this week, but I'm in no rush. :-) Just to make sure we're on the same page, this is the implementation of this Genesis proposal, and the specs and designs I linked above are what Alex sent us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
this is going to be fun to refactor with the new better performing withSubscriptions Higher Order Component now merged into dev to replace :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The star menu icon should have a hover Tooltip explaining what it does ("Redeem your rewards" or something like that).
This window should not have a fixed height, or large minHeight, as it does, rather should fit the required vertical space, with some maximum above which there appears a scroll bar. |
Could be nice if the star icon and red round circle thingy somehow could display the number of available redemptions. |
I don't think we need a star button that hover-expands to a Redeem button. There is plenty of space here, and it is easier for the user, to just have a "Redeem" button. |
When I click on "View All" with the Ganache 0 account, I just get an error message:
|
Bit of a layout problem here on mobile ("responsive" format). Could be this would be fixed by #1081 |
It took a bit to rebase, but that new HOC does make things quite a bit nicer! |
For these remarks, I suggest discussing with @A-Zak, since they weren't really specified in the original design.
Which account are you referring to?
In the interest of code reuse, I reused the ActionButton component that was already used in the proposal cards. I was under the assumption that if the GraphQL query would return it as a redeemable proposal, the redeem button would always show up. Can you think of a reason why that isn't the case there?
Right, I'll look into that.
I've just noticed that as well after a rebase. Could it be that this "connect" button while being logged in is something new? I thought being logged in and being connected to your wallet meant the same thing, but perhaps I was wrong in assuming that? What would the best solution be here UI-wise? |
@Procrat How about you get the latest from the dev branch where the mobile layout of the Connect button is fixed, then see what it looks like? Go from there... The term "Connect" is used when we have a cached current account, obtained from localStorage, but not a web3 provider for it that enables us to send txs on its behalf. |
@Procrat ganache |
@Procrat I will just refer you to the |
In the meantime, I've had a chat with Alex and received some UI mockups about the unspecified bits. I've rebased and
Since the button is on the left now, this is fortunately no longer an issue.
I'm afraid the preview app seems to be down. I'm still unclear on which account you're referring to, but perhaps seeing the preview app would have made it clear?
I was just curious whether you had an inkling about something that could have been easily overlooked. Anyway, looking at the ActionButton code, it shows the redeem button when Venturing into sugraph code, it looks like only Just to be clear, that GQL was taken from the existing per-DAO redemption page, so I would assume that whatever bug shows up here, is already present in the current version of Alchemy. Can you confirm that? |
I've
Apart from that issue that @dkent600 encountered (which I'm unclear on how to reproduce), are there any other issues left before this can be merged? |
I've added a tiny bit of robustness to I've encountered a bug with account 0x90f8bf6a... (one of the test accounts mentioned in the README) on the preview app, where this error was thrown: "Error: No action matching these callData could be found". After some digging, it looks like this is an issue with a particular DutchX proposal and this was apparently already an issue before this PR: see for example this other PR preview. |
hi @Procrat , could you make an issue for the preview app error and say a bit more about the circumstances? |
@Procrat So 1st of all thank you so much for the great work (and extra effort) - it looks really good :) 1st things 1st - ganache 0 account is the account with this private key
This happened on my personal account... To test this you can edit the
|
@A-Zak Thanks for the review! I didn't realise those accounts were common across projects using Ganache. Thanks for pointing that out. Those were the ones I've been testing with anyway. With regards to Alchemy crashing, that was the bug I had pointed out earlier. I've created an issue with my findings. I've rebased and fixed the other two issues you mentioned as well. Let me know if anything else needs to happen! |
Hey, @Procrat this looks really great thank you! :) Other than this everything is good to go! 💪 |
Also, it looks like there are a few conflicts to resolve... it'd be great if you could clear those as well 🙏 |
@A-Zak: Oh, I had only done it for the "redeem all" button in the menu and had forgotten about the other one. Should be fixed now! |
(Just another rebase) |
This looks great! :) |
return combineLatest(Array.from(daoIds, (id: any) => arc.dao(id).state())) | ||
.pipe(defaultIfEmpty([])); | ||
})); | ||
return combineLatest(daos, proposals); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any reason we actually need to to subscribe to the daos? i bet @jellegerbrandy has something to say about how we can reduce the number of subscriptions here given all the work he has been doing on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposal cards on the page require a DAO state as a prop. It looks like various subcomponents use it: ProposalData, ActionButton, VoteButtons, StakeButtons, AccountPopup, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks @Procrat ❤️
* WIP ProposalCard render props component to load proposal data in one place * Get ProposalData render props component working with ProposalCard Required allowing for async createObservable functions to be passed to withSubscriptions * Move all proposal subscriptions to ProposalData Don't move ProposalHistoryRow yet since it needs less data so moving would create more subscriptions * Better comment * Test/comment * Fix lint issues * Test commit * also use ProposalData warpper for proposalHistoryRow * update chrome version in wdio * Try to fix travis again * Revert "also use ProposalData warpper for proposalHistoryRow" This reverts commit 6784cdb. * Re-add redeem for beneficiary button to proposal details page (#1118) * Align headers of table in proposal history (#1117) * Align headers of table in proposal history * Fix merge issue * Links in proposal description should open in a new tab (#1120) * markdown links open in new tab * wait for good internet connection on startup (#1119) * correctly handle when initializeArc fails * loop until connected * add to changelog * less subscriptions and queries (#1114) * less subscriptions and queries * add new client dep * fix travis * remove environentvariables document * rename some dao to daoState * more dao -> daoState * Fix non-unique keys error on redeem button tooltip (#1126) https://daostack.tpondemand.com/RestUI/Board.aspx#page=board/5209716961861964288&appConfig=eyJhY2lkIjoiQjgzMTMzNDczNzlCMUI5QUE0RUE1NUVEOUQyQzdFNkIifQ==&boardPopup=bug/1510/silent * quick rename (#1127) * Upgrade eslint (#1121) * WIP * WIP * fix eslint config, correct lots of warnings * PR requests * restore sendSignedTransaction * add static mint and burn permission (#1131) * add static mint and burn permission * changelog * don't add Genesis Alpha on each render (#1132) * don't add Genesis Alpha on each render * changelog * changelog * changelog * display proper message when no history (#1130) * more robust handling of isLoading * revert render return type * repair merge errors * display msg when no history * changelog * lint * better return type for render (#1128) * WIP * WIP * fix eslint config, correct lots of warnings * PR requests * restore sendSignedTransaction * apply better return type to render calls * Cross-DAO Redemptions (#1047) * Remove unused CSS * Create page with all redemptions * Add a redemptions button to the header * Add redemptions quick menu for desktop users * Make Alchemy logo link to the DAOs page * Remove per DAO redemption button and page * Fix infinite scroll loading of queued scheme proposals (#1125) * Fix infinite scroll loading of queued scheme proposals There is one more bug to fix related to the time we use to check for when proposals expired, tricky one... https://daostack.tpondemand.com/RestUI/Board.aspx#page=board/5209716961861964288&appConfig=eyJhY2lkIjoiQjgzMTMzNDczNzlCMUI5QUE0RUE1NUVEOUQyQzdFNkIifQ==&boardPopup=bug/1639/silent * Fix bug when scheduling a proposal to timeout more than 24 days in the future It was thinking that the proposal timed out immediately! * changelog * Action buttons should appear when timer on proposal runs out (#1135) * call teardownSubscription * refactored comments * pass expired into ActionButton * add expired to ActionButton in RedemptionsMenu * changelog * integrate WalletConnect (#1104) * upgrade web3connect package * integrate WalletConnect * work around walletconnect issue * fix lint error * remove accidentlly commited file * upgrade web3connect * Login => Close, latest web3connect * changelog * changelog typo * trigger build * trigger build * generalize signing ex handling * restore the console message * merge from dev * fix merge * Release fixes (#1138) * fix error with non-recoganized callData * update client for quick fix * Release fixes (#1139) * fix error with non-recoganized callData * update client for quick fix * fix redemptions total * fix propals error * Release fixes (#1143) * fix error with non-recoganized callData * update client for quick fix * fix redemptions total * fix propals error * fix redemptions page
* ProposalCard render props component to load proposal data in one place * Get ProposalData render props component working with ProposalCard Required allowing for async createObservable functions to be passed to withSubscriptions * Move all proposal subscriptions to ProposalData Don't move ProposalHistoryRow yet since it needs less data so moving would create more subscriptions * Better comment * Test/comment * Fix lint issues * Test commit * also use ProposalData warpper for proposalHistoryRow * update chrome version in wdio * Try to fix travis again * Revert "also use ProposalData warpper for proposalHistoryRow" This reverts commit 6784cdb. * Re-add redeem for beneficiary button to proposal details page (#1118) * Align headers of table in proposal history (#1117) * Align headers of table in proposal history * Fix merge issue * Links in proposal description should open in a new tab (#1120) * markdown links open in new tab * wait for good internet connection on startup (#1119) * correctly handle when initializeArc fails * loop until connected * add to changelog * less subscriptions and queries (#1114) * less subscriptions and queries * add new client dep * fix travis * remove environentvariables document * rename some dao to daoState * more dao -> daoState * Fix non-unique keys error on redeem button tooltip (#1126) https://daostack.tpondemand.com/RestUI/Board.aspx#page=board/5209716961861964288&appConfig=eyJhY2lkIjoiQjgzMTMzNDczNzlCMUI5QUE0RUE1NUVEOUQyQzdFNkIifQ==&boardPopup=bug/1510/silent * quick rename (#1127) * Upgrade eslint (#1121) * WIP * WIP * fix eslint config, correct lots of warnings * PR requests * restore sendSignedTransaction * add static mint and burn permission (#1131) * add static mint and burn permission * changelog * don't add Genesis Alpha on each render (#1132) * don't add Genesis Alpha on each render * changelog * changelog * changelog * display proper message when no history (#1130) * more robust handling of isLoading * revert render return type * repair merge errors * display msg when no history * changelog * lint * better return type for render (#1128) * WIP * WIP * fix eslint config, correct lots of warnings * PR requests * restore sendSignedTransaction * apply better return type to render calls * Cross-DAO Redemptions (#1047) * Remove unused CSS * Create page with all redemptions * Add a redemptions button to the header * Add redemptions quick menu for desktop users * Make Alchemy logo link to the DAOs page * Remove per DAO redemption button and page * Fix infinite scroll loading of queued scheme proposals (#1125) * Fix infinite scroll loading of queued scheme proposals There is one more bug to fix related to the time we use to check for when proposals expired, tricky one... https://daostack.tpondemand.com/RestUI/Board.aspx#page=board/5209716961861964288&appConfig=eyJhY2lkIjoiQjgzMTMzNDczNzlCMUI5QUE0RUE1NUVEOUQyQzdFNkIifQ==&boardPopup=bug/1639/silent * Fix bug when scheduling a proposal to timeout more than 24 days in the future It was thinking that the proposal timed out immediately! * changelog * Action buttons should appear when timer on proposal runs out (#1135) * call teardownSubscription * refactored comments * pass expired into ActionButton * add expired to ActionButton in RedemptionsMenu * changelog * integrate WalletConnect (#1104) * upgrade web3connect package * integrate WalletConnect * work around walletconnect issue * fix lint error * remove accidentlly commited file * upgrade web3connect * Login => Close, latest web3connect * changelog * changelog typo * trigger build * trigger build * generalize signing ex handling * restore the console message * merge from dev * fix merge * Release fixes (#1138) * fix error with non-recoganized callData * update client for quick fix * Release fixes (#1139) * fix error with non-recoganized callData * update client for quick fix * fix redemptions total * fix propals error * Release fixes (#1143) * fix error with non-recoganized callData * update client for quick fix * fix redemptions total * fix propals error * fix redemptions page * refactor hamburger button (#1136) * refactoring * dev merge fixes * more tweaks * fix mobile menu vertical position * eliminate scrollbar on firefox * restore tour actions * fix merge errors * fix compile error * changelog * tweaks * revert styles * update changelog * Fix "Can't send transaction after changing account (#1141)" * detect when initializeArc has not been called * changelog * use console.error * ensure running is set to false * catch error * sum all stakes in history row (#1140) * sum all stakes * remove redundant variable * lot less subscriptions, upgrade client, use v28 of subgraph, prepare for switch to thegraph (#1134) * wip * intermediate commit, wip * tweaking the subscriptions * tweaks * use client 0.2.9 * trigger build * fix type error * clean up * remove a comment * upgrade client * thegraph providers * fix package-lock.json * fix merge confusion * subscribe to redemption updates * fix thegraph urls * more travis debug info , better docker file * add graphql_max_first setting to docker * update graph-node version * use latest test images * cargoculting * workaround graphql bug * fix typo * fix vulnerabilities * use client 0.2.12 * client 0.2.13; fix some vulenrabilites * fix syntax error * .. * hotfix * fix prod wss url * fix lint errors * use graphnode ipfs data * Compat design for DAO cards on mobile (#1145) * smoother transition to mobile * changelog * cleaner layout of statistics, reduce card height * tweak * Cached provider should always be used when possible (#1144) * remove .bind(this) * move cache-related functions to arc.ts * don't refetch cached provider if already using * all actions to use cached provider * don't show notification when using Connect button * changelog * add underscores to constants * collapse two calls to single enableWalletProvider * don't export setWeb3ProviderAndWarn * sort imports
This PR associates redemptions with the user, rather than with a DAO. The related design document can be found here.