-
Notifications
You must be signed in to change notification settings - Fork 5
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
POAP Dashboard App (Milestones 1-4) #2
Conversation
- basic layout - poap api bindings - network status - web3 status (xumm, gem wallet support) - mint page/dialog
I'll start reviewing this tomorrow :) |
See the section about [deployment](https://facebook.github.io/create-react-app/docs/deployment) for more information. | ||
|
||
## Supported Wallets | ||
- Xumm (installation details [here](https://xumm.app/)) |
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.
nit: Xumm is rebranding as Xaman
so maybe it'd be good to rename them in this project? (With a note like "Xaman, formerly named Xumm, ..."
Requires an updated version of the POAP backend
Update 1:New features:
Problems:
Misc:
|
My guess: The Notes:
|
- join dialog - claim request
- handle user does not exist - trim values
- tell the user to look at the wallet to confirm tx
- strong input/type checking - support for start and end date - make use of new POST /api/mint
Update 2:Frontend App changes:
Backend changes:
Info: |
Nevermind it was caused by the previous run of the code having a legacy database so it never re-initialized. Deleting |
Tried minting 12 NFTs with an event and the app crashed with this error: (I logged in with Xumm successfully before this using a Testnet account)
|
Looks like the exact same issue from before. Did you check the |
Ah sorry for the duplicate ask - you were right about the Currently I'm a bit confused about how event attendees would claim an NFT from the event. As it is now, it seems they would have to sign in via Xumm, but then they would be able to create new events which doesn't seem like something they should be able to do. (If everyday attendees have access to create events using the event organizer's secret, that means they can lock up the organizer's funds by minting a bunch of NFTs) There's a further challenge I'm noticing now that if you are an event organizer with a Xumm account, it's not compatible with the POAP backend because the backend expects a secret. Since Xumm accounts are initialized via secret numbers instead of secret you can't get the right data type for the I think the two work flows which have to be straightforward in this UI are:
Maybe the best way to go about that would be to have this UI as written so far just be for the event organizer, and have a button which creates a QR code to claim the NFT that can be shared with attendees? Then for login, potentially it would require verifying that the wallet that logged in was actually the same as the account on the backend & making the backend accept other forms of private key? @jonathanlei @rikublock what do you think? (Sorry for the wall of text - trying to figure out the best way to go about this) |
I imagined the dashboard app to be a flexible "Event Management Platform". Platform event types:
Platform user permissions:
Usage flow:
As I understand it, Attendify has a vault wallet ( Current dashboard implementation:
|
Changes:
(Changes to both frontend and backend) |
Sorry for the delay, I'll re-review tomorrow. |
NFT ownership verification is the key concern for me at the moment. Reaching a definitive decision on the way forward is critical, as it's currently impeding the progress. |
For the NFT ownership verification, I think Option 2 is best, and that should happen on the event page since it has a similar logic already, and will be a known url for attendees. While the NFTs can still be released, the page should say something like "You already have an NFT from this event." Once the event is over though, an end user who visits the site should be able to sign in with Xumm, and see a page which indicates "Verified! <r... (the account address)> owns an NFT from this event." with a big checkmark / easy to verify screen. If they log in, and they do not own an NFT from the event it should say something like "<r... (the account address> does NOT own an NFT from this event." with a red icon like a stop sign. Does that make sense? |
The following pages are intended for attendees (adjust masked event id):
That can be found at: http://localhost:3000/claim/4XyN519z
That can be found at: http://localhost:3000/verify/4XyN519z Let me know what you think. |
Did you have a chance to review it ? Would it be possible to compile a short list of all the remaining things that need changing? |
I had a chance to briefly check the specific thing previously. I'll do a full review tomorrow to verify all the requirements / try to provide very specific remaining feedback (if any). |
Once you claim your NFT, can you give names to the fields you display afterwards? (For someone who is unfamiliar with the XRPL, it'd be hard to know that you are displaying their wallet address and the NFTokenID) The verify page looks good. Can you link to it from the Other than that, the verify page looks great :) - Clear, simple, works well. I also verified that if there is an internet outage immediately after paying the reserve fee, or when canceling an event, the app handles the transactions when the internet comes back on gracefully (paying back the reserve and burning all NFTs in the cancel case). So I think the two small nits at the top of this comment are all that's left - I've updated the big checklist comment and all individual items are now checked off :) @rikublock - awesome work! |
I added a new icon at the top of the event page that links to the verification page. The event description might be quite long. Things at the bottom could be easily missed. In case you prefer text at the bottom of the page, can also add that.
Thanks! What happens now? What's next? |
@rikublock Sweet - I think it'd till be worth adding a note at the bottom right potentially with the words "Verify ownership" or something like that because I didn't quite expect that icon to be clickable since it looks like a stamp of authenticity rather than a link. But, this looks good to me. As for next steps:
(Thanks for sticking with it!) |
|
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.
LGTM, thanks for the work!
src/components/AttendeeTable.tsx
Outdated
type: "boolean", | ||
width: 80, | ||
}, | ||
// { |
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 this going to be used?
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.
It's actually not being used.
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.
could you delete the section then? Just to make sure the code is cleaning
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.
Certainly.
@rikublock Merged it in :) (If you want to make other changes, please do and @me, I'll aim to give quick reviews!) |
Nice! Might want to copy/fork https://github.com/rikublock/POAP-API2 as well. |
Proposal details see here
Discussion thread see here
Current Features:
Please checkout the README file for detailed setup instructions.
Will add more features in the coming days.
Feedback on the current state would be appreciated!
Mint Dialog:
Network/Wallet status: