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

Integrate WalletConnect #195

Merged
merged 34 commits into from
Sep 21, 2023
Merged

Integrate WalletConnect #195

merged 34 commits into from
Sep 21, 2023

Conversation

nop33
Copy link
Member

@nop33 nop33 commented Sep 1, 2023

No description provided.

@nop33 nop33 marked this pull request as draft September 1, 2023 09:38
@nop33 nop33 force-pushed the wc branch 2 times, most recently from 1096b6a to ef6e6ae Compare September 1, 2023 14:00
@nop33 nop33 force-pushed the wc branch 2 times, most recently from f20bcb6 to adb61a5 Compare September 1, 2023 14:09
<ButtonContainer key={i}>{child}</ButtonContainer>
))}
</View>
<View style={style}>{children?.map((child, i) => <ButtonContainer key={i}>{child}</ButtonContainer>)}</View>
Copy link
Member Author

Choose a reason for hiding this comment

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

Linting was broken for some reason so after upgrading the eslint related packages, this changes was required (also for next file)

@@ -174,6 +176,11 @@ const ScreenSectionCentered = styled(ScreenSection)`
align-items: center;
`

const TextContainer = styled.View`
width: 80%;
border: 0px solid transparent; // This is a hack cause I don't freaking understand why the text doesn't expand
Copy link
Member Author

Choose a reason for hiding this comment

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

😭

@nop33 nop33 force-pushed the wc branch 2 times, most recently from 361eb7a to 69ea4ea Compare September 1, 2023 14:37

const initializeWalletConnectClient = useCallback(async () => {
try {
console.log('⏳ INITIALIZING WC CLIENT...')
Copy link
Member Author

@nop33 nop33 Sep 1, 2023

Choose a reason for hiding this comment

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

I purposefully left many logs so that we're able to debug everything until the WC integration becomes stable. The logs are mainly before and after each call to WC, and when receiving an event from WC.

@nop33 nop33 force-pushed the wc branch 2 times, most recently from 3bcbb5c to ccafdea Compare September 1, 2023 14:52
@nop33 nop33 requested a review from mvaivre September 1, 2023 14:54
@nop33 nop33 changed the title WIP: Integrate WalletConnect Integrate WalletConnect Sep 1, 2023
@nop33 nop33 marked this pull request as ready for review September 1, 2023 14:55
Comment on lines +24 to +55
export type PendingTransaction =
| {
hash: string
fromAddress: string
toAddress: string
timestamp: number
amount?: string
tokens?: explorer.Token[]
lockTime?: number
status: 'pending'
type: 'transfer'
}
| {
hash: string
fromAddress: string
timestamp: number
amount?: string
tokens?: explorer.Token[]
lockTime?: number
status: 'pending'
type: 'call-contract'
}
| {
hash: string
fromAddress: string
timestamp: number
amount?: string
tokens?: explorer.Token[]
lockTime?: number
status: 'pending'
type: 'deploy-contract'
}
Copy link
Member

Choose a reason for hiding this comment

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

We could probably make this dryer, like:

type CommonPendingTransactionFields = {
  hash: string;
  fromAddress: string;
  timestamp: number;
  amount?: string;
  tokens?: explorer.Token[];
  lockTime?: number;
  status: 'pending';
};

type TransferPendingTransaction = CommonPendingTransactionFields & {
  toAddress: string;
  type: 'transfer';
};

type CallContractPendingTransaction = CommonPendingTransactionFields & {
  type: 'call-contract';
};

type DeployContractPendingTransaction = CommonPendingTransactionFields & {
  type: 'deploy-contract';
};

export type PendingTransaction =
  | TransferTransaction
  | CallContractTransaction
  | DeployContractTransaction;

@@ -0,0 +1,714 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Okay.. I can imagine that this one was pain

Copy link
Member

Choose a reason for hiding this comment

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

This file seems to be a rather "simple" in terms of code (mostly conditions and state update)... I feel like all we need now is a lot of testing.

[walletConnectClient]
)

const respondToWalletConnectWithSuccess = async (event: SessionRequestEvent, result: SignResult) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this one wrap in a useCallback call?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's only used by handleApprovePress which is not used in a dependency array of anything, so there's no need to wrap with a useCallback.

@nop33 nop33 requested a review from mvaivre September 19, 2023 15:47
@nop33
Copy link
Member Author

nop33 commented Sep 19, 2023

Phew, that merge conflict resolution was a pain in the 🍑, mainly because in the meantime we changed the modals from Modalize to our own. Needs a lot of testing. Hopefully we can merge this before we add many things on master 😅

Copy link
Member

Choose a reason for hiding this comment

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

Mhh is that good?

async (uri: string) => {
if (!walletConnectClient) return

const pairingTopic = uri.substring('wc:'.length, uri.indexOf('@'))
Copy link
Member

Choose a reason for hiding this comment

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

🤓

Interesting idea the 'wc:'.length to make it more explicit for the reader than just "3"

@mvaivre
Copy link
Member

mvaivre commented Sep 20, 2023

This is some heavy stuff sir

@mvaivre
Copy link
Member

mvaivre commented Sep 20, 2023

My propositions:

  • Switch the address that is used: open a bottom modal on top (feasible?) to select an address on the fly
  • Highlight the connection button using accent color

@nop33 nop33 requested a review from mvaivre September 20, 2023 16:08
@nop33
Copy link
Member Author

nop33 commented Sep 20, 2023

Switch the address that is used: open a bottom modal on top (feasible?) to select an address on the fly

It's quite tricky to implement that, I would need to lift the state of the proposal modal up to the context and hack it through, it didn't feel clean at all. Since the modal works well when its content is changed, I'd rather leave it as it is for now (I improved the layout a bit)

@nop33 nop33 merged commit 907c76d into master Sep 21, 2023
2 checks 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.

2 participants