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

[Add App] Sky.money #412

Open
8 tasks done
zdumitru opened this issue Nov 7, 2024 · 14 comments
Open
8 tasks done

[Add App] Sky.money #412

zdumitru opened this issue Nov 7, 2024 · 14 comments
Labels
Waiting for Owner The submission is awaiting a response from the owner.

Comments

@zdumitru
Copy link

zdumitru commented Nov 7, 2024

Entry type

  • New addition

App info

URL: https://app.sky.money/

Manifest.json URL: https://app.sky.money/manifest.json

Name: sky.money

Description: Rewards, savings, upgrade, and trade

Icon (PNG, 180x180):
sky_logo

It's minified via https://tinypng.com: yes

Homepage: https://sky.money/
Twitter: https://x.com/SkyEcosystem
GitHub: https://github.com/skybase-int
Discord: https://discord.gg/skyecosystem

App supports batching multiple transactions via Safe: no

Supported networks

- Mainnet

Revision checks

  • Used smart contracts were audited.
  • You have implemented the app using the Safe Apps SDK
  • Your Safe App includes a manifest.json file at the root with the required data
  • The app can be loaded as a custom Safe App in the Apps section of https://app.safe.global.
  • The app auto-connects to the Safe as a wallet
  • It doesn't try to connect to the browser wallet (e.g. MetaMask)
  • You are able to trigger and execute one transaction with a Safe.
  • RPC requests are optimized (not triggering many requests in a very short time period).

Audit document

https://www.chainsecurity.com/smart-contract-audit-reports?client=MakerDao

Code for review

https://github.com/skybase-int/webapp

Team information

Company: TechOps Services

Official website: https://techops.services/

Point of contact: Dumitru

Email/Telegram: contact[at]techops.services

@kirkkonen
Copy link

This app was reviewed and approved by the product team.

@zdumitru
Copy link
Author

zdumitru commented Dec 3, 2024

This app was reviewed and approved by the product team.

Hi @kirkkonen ,
Thanks for approving the app. Can you please share when the app is going to be live? Currently it's not visible in the gnosis safe app list. Thank you!

@PooyaRaki
Copy link
Contributor

This app was reviewed and approved by the product team.

Hi @kirkkonen , Thanks for approving the app. Can you please share when the app is going to be live? Currently it's not visible in the gnosis safe app list. Thank you!

Hi @zdumitru
Thank you for your submission! The tech team will first review the code and the audit results, followed by the QA team’s review. Once all reviews are complete, we will proceed to list the app.

@iamacook
Copy link
Member

@zdumitru, I’ve reviewed the shared repository, and everything looks good so far. However, I’m unable to find the transaction code. If I understand correctly, it’s handled within the @jetstreamgg/widgets package? Could you grant me access to its code so we can continue the review?

@zdumitru
Copy link
Author

zdumitru commented Dec 10, 2024

@zdumitru, I’ve reviewed the shared repository, and everything looks good so far. However, I’m unable to find the transaction code. If I understand correctly, it’s handled within the @jetstreamgg/widgets package? Could you grant me access to its code so we can continue the review?

Read access given. That repo will be open sourced some time soon.

@iamacook
Copy link
Member

Despite the lack of test coverage, I did not encounter any critical issues with the submission. I’m happy to pass it on to QA for further review. cc @francovenica

@github-project-automation github-project-automation bot moved this to New issues in Safe{Wallet} Dec 10, 2024
@liliya-soroka liliya-soroka moved this from New issues to Ready for QA in Safe{Wallet} Dec 10, 2024
@francovenica francovenica moved this from Ready for QA to QA in progress in Safe{Wallet} Dec 18, 2024
@francovenica
Copy link

francovenica commented Dec 20, 2024

1 -Minor issue. The "Copy address" button doesn't work. Clicking on it won't save the address in my clipboard
image

2 - Major issue:
I tried a trade transaction, USDC for DAI, and I was able to sign and execute the approval, but the app "Doesn't notice" that that tx was successfully executed so I'm stuck waiting for the other tx (The trade itself) to show up
image

I had this same issue trying an upgrade from DAI to USDS; Tx executed fine, but the app gets stuck waiting for the upgrade to finish

3 - Major issue:
Having the approved I refreshed the app so I can try the trade of USDC for DAI. I signed the message successfully, but the trade still fails.
image

@francovenica francovenica moved this from QA in progress to Ready for QA in Safe{Wallet} Dec 20, 2024
@liliya-soroka liliya-soroka moved this from Ready for QA to Todo in Safe{Wallet} Dec 27, 2024
@PooyaRaki
Copy link
Contributor

@zdumitru We are awaiting your response to proceed with the process.

@zdumitru
Copy link
Author

@zdumitru We are awaiting your response to proceed with the process.

Thanks. We'll address the issue and fix is shortly. Will come back to you when done.

@PooyaRaki
Copy link
Contributor

@zdumitru Thanks for the update, I’m closing this issue for now.
Feel free to reopen it with an update whenever the fix is ready.

@github-project-automation github-project-automation bot moved this from Todo to Done in Safe{Wallet} Jan 3, 2025
@PooyaRaki PooyaRaki added the Waiting for Owner The submission is awaiting a response from the owner. label Jan 11, 2025
@zdumitru
Copy link
Author

@PooyaRaki I'm afraid I cannot reopen the issue myself. Please help me here.

We updated all what you asked and pushed to production a new version with some fixes to the issues mentioned.

For the issues reported:
2 and 3. Are part of a known issue, but we weren't handling it properly. Due to the way CoWswap works, trades from smart contract wallets are currently not supported in the app. This is a coming soon feature, but for now we've disabled the ability to perform trade transactions from Safe wallets along with a message explaining this, so users are not left hanging indefinitely

  1. This seems to be an issue with the wallet provider package we use (RainbowKit) that is only affecting Safe wallets. Hopefully Rainbow fixes this soon on their end, but fixing it on our end would involve switching the provider package we use. Since this was labeled as a minor issue, could you ask them if it's required for us to fix for the app to be added to the Safe apps list? We do provide the copy address to clipboard functionality in the Balances widget (https://app.sky.money/?widget=balances)

RE: The issue reported when upgrading DAI to USDS. We tested this several times using test Safe wallets and couldn't reproduce the issue. I made this screen recording to show how I tested it. Could you ask them to try again and maybe provide the steps to reproduce if they can still experience the issue? - https://we.tl/t-eGfZjEiopT

Let us know if that helps or if any more details are needed. Thanks in advance!

@PooyaRaki PooyaRaki reopened this Jan 20, 2025
@PooyaRaki
Copy link
Contributor

@zdumitru I'm reopening the issue and sending it to Q.A for another review.

@PooyaRaki PooyaRaki added Ready for Q.A The submission is ready for Q.A. review. and removed Waiting for Owner The submission is awaiting a response from the owner. labels Jan 20, 2025
@francovenica
Copy link

francovenica commented Jan 21, 2025

  • We had recent reports of the Rainbowkit not working properly for this copy feature, so we can ignore it.
  • The swap feature is disabled for the Safe, with the message present there. This is correct and passes
  • Regarding the upgrade. I tried again, upgrading DAI to to USDS and this time I didn't had the error I mentioned in the previous comment.
    What it did happen is the same issue I reported before with the Swap: the app doesn't change its state and "waits" for the tx to be done, when it was already done several minutes ago. I waited around 5 mins but never updated
    Is this the same problem as what is was mentioned for the Swap feature?

Here is a gif (if the gif is jumpy is because I edited it to be shorter). Here you can see that I wait and the tx state never updates. Is not until I exit an enter again that the state is the one I expect
Image

This is the tx I executed
https://app.safe.global/transactions/tx?safe=eth:0x8675B754342754A30A2AeF474D114d8460bca19b&id=multisig_0x8675B754342754A30A2AeF474D114d8460bca19b_0x3655714a61641dda103a2c61fd3e7ca3ab5b74e3afc7cc07c7d8a99187ad9da7

@francovenica francovenica added Waiting for Owner The submission is awaiting a response from the owner. and removed Ready for Q.A The submission is ready for Q.A. review. labels Jan 21, 2025
@francovenica
Copy link

Note, my team suggested for you to check if the app is waiting for the "TransactionHash" or the "SafeTxHash". It should wait for the latter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting for Owner The submission is awaiting a response from the owner.
Projects
Archived in project
Development

No branches or pull requests

5 participants