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 access to approve session properties #271

Merged

Conversation

hmendez-figure
Copy link
Contributor

@hmendez-figure hmendez-figure commented Feb 22, 2024

Description

Allows full access to SignEngine.approveSession (specifically sessionProperties) params from interfaces.

dApp and wallet are able to send optional sessionProperties via Proposal and Settlement messages according to spec. Currently the dApp client exposes this via connect() but wallet client does not in approveSession even though SignEngine.approveSession in fact does implement it. Though the interface (ISignEngineWallet) does not expose it.

I compared this to the JS sign client and it also exposes it here

Resolves # (issue)

How Has This Been Tested?

Smoke Test

Due Dilligence

  • Breaking change
  • Requires a documentation update

@quetool
Copy link
Collaborator

quetool commented Feb 23, 2024

Hello @hmendez-figure, thanks for opening! I'll check this PR as soon as I can, in the meantime could you add in the description and explanation on why is this needed? Thanks!

Copy link

sonarcloud bot commented Feb 26, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Apr 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@VigM-Figure
Copy link

Hi @quetool, was there anything else needed or any concerns holding up this pull request? Our organization has taken to forking the repo to solve the problem about not having access to sessionProperties but would ideally like to revert and stay on the main branch/repo if possible. Thanks!

@quetool
Copy link
Collaborator

quetool commented Apr 16, 2024

Hello @VigM-Figure! Actually thanks for the ping! I will take care of this soon! Thanks!

@quetool quetool self-requested a review April 17, 2024 17:05
@quetool quetool self-assigned this Apr 17, 2024
@quetool
Copy link
Collaborator

quetool commented Apr 17, 2024

Hello @hmendez-figure, do you mind trying test on your end and fix then if any? Thanks!

@quetool quetool merged commit 9c0e03b into WalletConnect:master Apr 17, 2024
1 of 2 checks passed
@quetool
Copy link
Collaborator

quetool commented Apr 17, 2024

Never mind! I will just merge it and take care of it on my end! Thanks @hmendez-figure @VigM-Figure for this and sorry for the huge delay! It will be deployed on the next release! 👌

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.

3 participants