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

[roles-0.2.0/streams] - Now the payroll is getting real baby #2156

Conversation

mudrila
Copy link
Contributor

@mudrila mudrila commented Jul 18, 2024

This PR adds ability to add a payroll stream to an existing hat or newly added hat when tree is present
That means it will only work when roles were added before!

Testing:

  1. Create brand new Safe. Fund it with some ERC-20 to use for Payroll.
  2. Create a proposal to initialize Hats Tree (aka create 1 hat without a payroll)
  3. Execute that proposal
  4. Edit that hat - add a payroll for that, create proposal. Remember step 1 - you'll need some ERC-20 in the treasury! You can't stream native token (ETH). You should see updated payroll details in the edited hats table
  5. Make that proposal pass and execute it.
  6. Make sure proper amount of ERC-20 tokens were sent from Safe's treasury
  7. Now the tricky part - we need to verify that stream was created with proper values etc. For that - we need to go into executed transaction on Etherscan, find the event with stream creation, grab the recipient address and verify that it corresponds with the Hat Account assigned to that Hat.

@mudrila mudrila added the enhancement New feature or request label Jul 18, 2024
@mudrila mudrila self-assigned this Jul 18, 2024
Copy link

netlify bot commented Jul 18, 2024

Deploy Preview for decent-interface-dev ready!

Name Link
🔨 Latest commit bc71740
🔍 Latest deploy log https://app.netlify.com/sites/decent-interface-dev/deploys/6698c22826e5d400080fc219
😎 Deploy Preview https://deploy-preview-2156.app.dev.decentdao.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tomstuart123
Copy link

Awesome @mudrila . Buzzed to test

I'm at step#3 but blocked by moralis netlify key needing updated. Request to update keys here. Once done, I'll finish the test

@tomstuart123
Copy link

Awesome @mudrila so good seeing this all linking up.

Primary feedback on logic/tests:

  • Create brand new Safe. Fund it with some ERC-20 to use for Payroll. ✅
  • Create a proposal to initialize Hats Tree (aka create 1 hat without a payroll)✅
  • Execute that proposal✅
  • Edit that hat - add a payroll for that, create proposal. Remember step 1 - you'll need some ERC-20 in the treasury! You can't stream native token (ETH). You should see updated payroll details in the edited hats table✅
  • Make that proposal pass and execute it.✅
  • Make sure proper amount of ERC-20 tokens were sent from Safe's treasury✅
  • Now the tricky part - we need to verify that stream was created with proper values etc. For that - we need to go into executed transaction on Etherscan, find the event with stream creation, grab the recipient address and verify that it corresponds with the Hat Account assigned to that Hat. ✅

Note @decentdao/engineering could someone also double check my thinking for why I think the last bullet point is a ✅

  • Reasoning -
    --> See below screenshot from event logs for Hat Role creation. ✅
    Screenshot 2024-07-18 at 12 02 42

--> 0xfC5Fd67C8C9e478D029827175230BEB883Bb9c09 this appears to be the hataccount for hatID '10460459718236864672367699073269748130132045948045810886507630648360960'
--> If you compare it to this screenshot, it defines 0xfC5Fd67C8C9e478D029827175230BEB883Bb9c09 as the recipient of the stream
Screenshot 2024-07-18 at 12 03 39

Copy link

@tomstuart123 tomstuart123 left a comment

Choose a reason for hiding this comment

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

@mudrila approving this for now as all the logic seems to be working :). Great job

Note @decentdao/design / @decentdao/engineering - I do have a number of product/design feedback that I'm logging and will write up in the root 'streams' PR pre we deploy to dev

For now though, lets just keep heads down.

@tomstuart123
Copy link

Discussion at standup to merge now. @mudrila can you merge

Copy link
Contributor

@DarksightKellar DarksightKellar left a comment

Choose a reason for hiding this comment

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

Lookin' good. Minor comments but approving anyway in lieu of standup decision to move things forward

src/hooks/schemas/roles/useRolesSchema.ts Show resolved Hide resolved
src/hooks/streams/useCreateSablierStream.ts Show resolved Hide resolved
@mudrila mudrila merged commit 208c1e7 into roles-0.2.0/streams Jul 18, 2024
7 checks passed
@mudrila mudrila deleted the roles-0.2.0/now-this-shit-with-payroll-getting-real branch July 18, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants