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

Feature: Relay getTxBySafeTxHash via interface, use client gateway #180

Merged
merged 10 commits into from
Jun 8, 2021

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Jun 2, 2021

This PR:

@mmv08 mmv08 requested review from germartinez and katspaugh June 2, 2021 15:08
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2021

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 19 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@mmv08
Copy link
Member Author

mmv08 commented Jun 2, 2021

Depends on 5afe/safe-react#2361

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Looks good! Just some minor comments.

Is there any way to not show the dist files in PRs? Committing the dist after the review maybe?

packages/safe-apps-sdk/src/txs/index.ts Outdated Show resolved Hide resolved
packages/safe-apps-sdk/src/txs/index.ts Outdated Show resolved Hide resolved
packages/safe-apps-sdk/src/txs/index.ts Outdated Show resolved Hide resolved
import { SafeInfo } from './sdk';
import { GatewayTransactionDetails, SafeBalances } from './gateway';

export type Methods = keyof typeof METHODS;
Copy link
Member

Choose a reason for hiding this comment

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

Not related to the PR directly but... you wouldn't need this if METHODS was an enum.

Copy link
Member Author

@mmv08 mmv08 Jun 3, 2021

Choose a reason for hiding this comment

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

In general types for this SDK are messy and there's room for big improvement I believe. The main thing I don't understand how to do properly is to have a connection between method/params/responses in both public and private methods. This is the reason for cumbersome constructions such as:

  public send = <M extends Methods, P, R>(method: M, params: P): Promise<Response<R>> => {

If you want/have time to look at it and see if you have an idea, would really appreciate it 🙂

@mmv08 mmv08 requested a review from katspaugh June 3, 2021 15:09
@mmv08 mmv08 merged commit 54528c7 into development Jun 8, 2021
@mmv08 mmv08 deleted the feature/getBySafeTxHash-new-endpoint branch June 8, 2021 08:50
@github-actions github-actions bot locked and limited conversation to collaborators Jun 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants