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

Onboarding wizard #77

Merged
merged 14 commits into from
Jul 10, 2023
Merged

Onboarding wizard #77

merged 14 commits into from
Jul 10, 2023

Conversation

Venoox
Copy link
Contributor

@Venoox Venoox commented Jun 21, 2023

Resolves #60

@Venoox Venoox marked this pull request as ready for review June 25, 2023 13:19
@Venoox Venoox requested a review from 0x4007 as a code owner June 25, 2023 13:19
@Venoox Venoox changed the base branch from main to development June 25, 2023 13:28
@0x4007
Copy link
Member

0x4007 commented Jun 26, 2023

@rndquu's continuous deployment fix will also allow us to complete this review, thank you for your patience.

@rndquu
Copy link
Member

rndquu commented Jun 27, 2023

@Venoox pls merge the latest development branch

@Venoox
Copy link
Contributor Author

Venoox commented Jun 27, 2023

@rndquu I think the pull request workflow is executing in my fork's context so secrets are not available there. I think if you want to execute workflow in the original repo's context you need to use pull_request_target

@rndquu
Copy link
Member

rndquu commented Jun 27, 2023

@rndquu I think the pull request workflow is executing in my fork's context so secrets are not available there. I think if you want to execute workflow in the original repo's context you need to use pull_request_target

I don't get then why it works in the other repo

@0x4007
Copy link
Member

0x4007 commented Jun 27, 2023

@rndquu I think the pull request workflow is executing in my fork's context so secrets are not available there. I think if you want to execute workflow in the original repo's context you need to use pull_request_target

I don't get then why it works in the other repo

I think posting as ubiquibot is privileged (getting its access token) perhaps try without the ubiquibot step

@rndquu
Copy link
Member

rndquu commented Jun 29, 2023

@Venoox pls merge the latest development branch

think the pull request workflow is executing in my fork's context so secrets are not available there. I think if you want to execute workflow in the original repo's context you need to use pull_request_target

Yes, secrets are not available because this PR is created from a forked repo. However using pull_request_target with PR checkout allows malicious PRs to exfiltrate secrets so pull_request_target should be used with caution.

The reason why similar setup (forked repo + cloudflare deploy) works in our other repo is because when workflow is executed on workflow_run it is run in the context of the target branch (not PR) so it has access to secrets.

@rndquu
Copy link
Member

rndquu commented Jun 29, 2023

@pavlovcik I'm still working on the "post deploy URL" comment feature but if you look at actions you will find the "Cloudflare Preview Deployment" which prints deploy URL

this PR can be checked here: https://a063b2de.pay-ubq-fi-bxp.pages.dev/

@rndquu
Copy link
Member

rndquu commented Jun 29, 2023

@Venoox one more time pls merge the development branch

@rndquu
Copy link
Member

rndquu commented Jul 5, 2023

@Venoox pls fix the conflicts

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

@0x4007
Copy link
Member

0x4007 commented Jul 6, 2023

@Venoox one more time pls merge the development branch

I think you have the ability to access this pull request from your local vscode client and make changes and push by the way.

I use the gh commands to check out the pull request.

gh pr checkout 77

@rndquu rndquu self-requested a review July 7, 2023 07:03
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

  1. If gnosis chain is selected on step 1 then on step 2 on "Approve" button click nothing happens and there is this error in the console:
onboarding.ts:344 Uncaught TypeError: Cannot set properties of null (setting 'innerHTML')
    at HTMLInputElement.<anonymous> (onboarding.ts:344:112)
(anonymous) @ onboarding.ts:344
onboarding.ts:501 Error: call revert exception [ See: https://links.ethers.org/v5-errors-CALL_EXCEPTION ] (method="decimals()", data="0x", errorArgs=null, errorName=null, errorSignature=null, reason=null, code=CALL_EXCEPTION, version=abi/5.7.0)
    at Logger21.makeError (index.ts:269:28)
    at Logger21.throwError (index.ts:281:20)
    at Interface3.decodeFunctionResult (interface.ts:427:23)
    at Contract3.<anonymous> (index.ts:400:44)
    at step (index.ts:1:1)
    at Object.next (index.ts:1:1)
    at fulfilled (index.ts:1:1)

Works fine for ethereum mainnet though.

  1. Pls add input field validation:
  • wallet private key should be in the correct format (i.e. 32 bytes length)
  • safe address should be a valid ethereum address
  • github PAT should not be empty
  • org name should not be empty
  • allowance amount should be > 0

@Venoox Venoox requested a review from rndquu July 10, 2023 09:06
@github-actions
Copy link
Contributor

Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

If you open https://ac81444b.pay-ubq-fi-bxp.pages.dev/onboarding and leave all of the fields empty, then on "Set" click step 2 is opened. Expected behaviour is that some validation error is shown because all of the fields are empty.

@Venoox
Copy link
Contributor Author

Venoox commented Jul 10, 2023

I forgot to remove the code that skips first step because I was testing it.

@github-actions
Copy link
Contributor

@0x4007 0x4007 merged commit 2548cdc into ubiquity:development Jul 10, 2023
1 check passed
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.

Partner Onboarding: Allow Permit2 Spender in Custom UI
3 participants