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.

Sign up for GitHub

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

Jump to bottom

TRM labs integration #923

Merged
merged 30 commits into from Aug 11, 2022
Merged

TRM labs integration #923

merged 30 commits into from Aug 11, 2022

Conversation

piekczyk
Copy link
Collaborator

@piekczyk piekczyk commented Mar 30, 2022 •
edited

TRM labs integration

Changes 👷‍♀️

  • added risk verification after ToS flow

How to test 🧪

  • use different kind of wallets and addresses and check whether risk is verified within network tab
  • check in db whether additional information are stored in tos_approval (chain_id, signature, message)
  • on goerli wallet_risk table in db shouldn't be updated as for test network we always return isRisky: false
  • on mainnet wallet_risk table should be updated

Definition of done ✔️

  • Acceptance criteria for each issue met
  • Unit tests written where needed and passing
  • End-to-end tests for happy path
  • Documentation updated
  • Project builds without errors
  • Non-functional requirements met
  • Code reviewed and functionality tested by the reviewer (locally, Heroku, etc.)
  • Feature verified and accepted by Product Owner
  • Project deployed to production environment

@paszkowskiDamian paszkowskiDamian temporarily deployed to oasis-app-sp-trm-labs-i-nnjk7u March 30, 2022 16:21 Inactive
@paszkowskiDamian paszkowskiDamian temporarily deployed to oasis-app-sp-trm-labs-i-tcpnhm March 31, 2022 06:03 Inactive
@codecov
Copy link

codecov bot commented Mar 31, 2022 •
edited

Codecov Report

Merging #923 (8397949) into dev (9031556) will increase coverage by 0.04%.
The diff coverage is 41.46%.

@@            Coverage Diff             @@
##              dev     #923      +/-   ##
==========================================
+ Coverage   62.55%   62.60%   +0.04%     
==========================================
  Files         225      226       +1     
  Lines        6927     6936       +9     
  Branches     1651     1651              
==========================================
+ Hits         4333     4342       +9     
- Misses       2233     2234       +1     
+ Partials      361      360       -1
Impacted Files Coverage Δ
features/termsOfService/jwt.ts 30.76% <15.38%> (-3.72%) ⬇️
features/walletAssociatedRisk/walletRisk.ts 40.00% <40.00%> (ø)
features/walletAssociatedRisk/walletRiskApi.ts 55.55% <55.55%> (ø)
features/termsOfService/termsAcceptance.ts 97.95% <100.00%> (+25.23%) ⬆️
features/termsOfService/termAcceptanceLocal.ts

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@paszkowskiDamian paszkowskiDamian temporarily deployed to oasis-app-sp-trm-labs-i-kcocsk March 31, 2022 08:30 Inactive
@paszkowskiDamian paszkowskiDamian temporarily deployed to oasis-app-sp-trm-labs-i-ixopbo March 31, 2022 08:49 Inactive
@adamskrodzki adamskrodzki had a problem deploying to my-test-sp-trm-labs-int-dswrib April 1, 2022 09:05 Failure
@paszkowskiDamian paszkowskiDamian had a problem deploying to oasis-app-sp-trm-labs-i-pzpiq6 April 1, 2022 09:05 Failure
@paszkowskiDamian paszkowskiDamian temporarily deployed to oasis-app-sp-trm-labs-i-dpacce April 1, 2022 10:00 Inactive
@adamskrodzki adamskrodzki temporarily deployed to my-test-sp-trm-labs-int-zhecqi April 1, 2022 10:00 Inactive
@piekczyk piekczyk changed the title [WIP] TRM labs integration TRM labs integration Apr 1, 2022
@paszkowskiDamian paszkowskiDamian temporarily deployed to oasis-app-sp-trm-labs-i-oaes2j April 1, 2022 11:48 Inactive
johnnieskywalker
johnnieskywalker reviewed Apr 6, 2022
View reviewed changes
Copy link
Contributor

@johnnieskywalker johnnieskywalker left a comment

Choose a reason for hiding this comment

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

Shouldn't this feature be controlled by flag in features ?

@johnnieskywalker
Copy link
Contributor

Shouldn't this feature be controlled by flag in features ?

Changed my mind, this would be serious security vulnerability

@vercel
Copy link

vercel bot commented Apr 6, 2022 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oazo-test/oasis-borrow-test/81qXHMpyBtYb6hs72gwjNnkef6gg
✅ Preview: Failed

[Deployment for fdea680 failed]

@piekczyk piekczyk changed the title [DONT MERGE] TRM labs integration TRM labs integration Jul 25, 2022
adamskrodzki
adamskrodzki previously approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@adamskrodzki adamskrodzki left a comment

Choose a reason for hiding this comment

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

tested and works

features/termsOfService/jwt.ts
@@ -12,24 +13,40 @@ export type JWToken = string

export function jwtAuthGetToken(address: string): JWToken | undefined {
const token = localStorage.getItem(`token-b/${address}`)
if (token && token !== 'xxx') {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is magic 'xxx' ?

Copy link
Contributor

Choose a reason for hiding this comment

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

test what happens if user override locally token to xxx

features/walletAssociatedRisk/WalletAssociatedRisk.tsx
children: ReactNode
}

export function WithWalletAssociatedRisk({ children }: WithWalletAssociatedRiskProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why this control is used multiple times instead of being used on the top once in _app.tsx?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

due to dependency on ToS flow

features/walletAssociatedRisk/walletRiskApi.ts
error?: string
}

export function getWalletRisk$(token: string, chainId: number): Observable<WalletRiskResponse> {
Copy link
Contributor

Choose a reason for hiding this comment

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

why it is not wallet address dependent ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

address is decoded from token

@piekczyk
Merge branch 'dev' into sp/trm-labs-integration
8397949 
# Conflicts:
#	pages/owner/[address]/index.tsx
@sonarcloud
Copy link

sonarcloud bot commented Aug 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@piekczyk piekczyk merged commit ef5b3e1 into dev Aug 11, 2022
@piekczyk piekczyk deleted the sp/trm-labs-integration branch August 11, 2022 09:23
@paulmillr
Copy link

@adamskrodzki @paszkowskiDamian @piekczyk why are you doing this? Do you live in the U.S.?

Bootlicking did not help Tornado. They had compliance section and still got banned. So what's the purpose?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamskrodzki adamskrodzki adamskrodzki approved these changes

@paszkowskiDamian paszkowskiDamian paszkowskiDamian approved these changes

@johnnieskywalker johnnieskywalker Awaiting requested review from johnnieskywalker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@piekczyk @johnnieskywalker @paulmillr @adamskrodzki @paszkowskiDamian @halaprix