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

fix: synced expiries #4159

Merged
merged 27 commits into from
Jan 26, 2024
Merged

fix: synced expiries #4159

merged 27 commits into from
Jan 26, 2024

Conversation

ganchoradkov
Copy link
Member

@ganchoradkov ganchoradkov commented Jan 22, 2024

Description

Implemented synced expiries for session proposals and session requests. Synced expiries are defined by the sending peer and used by both sending & receiving peers. If no expiryTimestamp is received, default values are used.

Additionally

  • patched a few flaky tests to always await async requests
  • increased publish<->ack timeout to 20s
  • implemented mechanism to wait for all requests in flight (that wait for a relay ack) to complete before closing transport
  • implemented cleanup on sent pending requests (history store) when the session is disconnected

Type of change

  • Chore (non-breaking change that addresses non-functional tasks, maintenance, or code quality improvements)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Draft PR (breaking/non-breaking change which needs more work for having a proper functionality [Mark this PR as ready to review only when completely ready])
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

tests
dogfooding
WalletConnect/web-examples#432

Checklist

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Additional Information (Optional)

Please include any additional information that may be useful for the reviewer.

@ganchoradkov ganchoradkov changed the title fix: implements synced expiries for proposals and session request fix: synced expiries Jan 22, 2024
@arein arein added the accepted label Jan 22, 2024
@ganchoradkov ganchoradkov marked this pull request as ready for review January 26, 2024 08:07
@bkrem
Copy link
Member

bkrem commented Jan 26, 2024

If no expiryTimestamp is received, default values are used.

This is for backwards compat I'm assuming?

@ganchoradkov
Copy link
Member Author

This is for backwards compat I'm assuming?

correct

Copy link
Member

@bkrem bkrem left a comment

Choose a reason for hiding this comment

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

Nice 💯 some minor tweaks suggested but LGTM overall

@@ -156,6 +157,8 @@ export class Engine extends IEngine {

const publicKey = await this.client.core.crypto.generateKeyPair();

const expiry = ENGINE_RPC_OPTS.wc_sessionPropose.req.ttl || FIVE_MINUTES;
Copy link
Member

Choose a reason for hiding this comment

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

ENGINE_RPC_OPTS.wc_sessionPropose.req.ttl is already a constant right, why do we need the || defaulting?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's type is actually number | undefined causing ts to complain

Copy link
Member Author

Choose a reason for hiding this comment

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

might be better just updating it to explicit type number and not optional

@@ -397,6 +398,16 @@ export class Pairing implements IPairing {
const { message } = getInternalError("MISSING_OR_INVALID", `pair() uri#symKey`);
throw new Error(message);
}
if (uri?.expiryTimestamp) {
const expiration = toMiliseconds(uri?.expiryTimestamp);
if (expiration < Date.now()) {
Copy link
Member

Choose a reason for hiding this comment

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

Probably strictly theoretical rather than realistic, but I think there's an edge case here where:

  • toMiliseconds simply adds zeroes to the end of seconds-based expiration
  • Date.now() has non-zero values in the last 3 digits

Either way I think we can ignore because that only applies within the same second :)

packages/core/src/controllers/publisher.ts Outdated Show resolved Hide resolved
packages/sign-client/src/controllers/engine.ts Outdated Show resolved Hide resolved
packages/sign-client/src/controllers/engine.ts Outdated Show resolved Hide resolved
packages/sign-client/src/controllers/engine.ts Outdated Show resolved Hide resolved
packages/sign-client/src/controllers/engine.ts Outdated Show resolved Hide resolved
packages/types/src/sign-client/proposal.ts Outdated Show resolved Hide resolved
packages/core/src/controllers/relayer.ts Show resolved Hide resolved
packages/sign-client/src/controllers/engine.ts Outdated Show resolved Hide resolved
@ganchoradkov ganchoradkov merged commit 51ec49e into v2.0 Jan 26, 2024
9 checks passed
@ganchoradkov ganchoradkov deleted the fix/synced-expiries branch January 26, 2024 15:24
@ganchoradkov ganchoradkov mentioned this pull request Feb 6, 2024
20 tasks
@kieranbop1990
Copy link

@ganchoradkov it's not entirely clear how to apply this fix to override those five minutes. Any documentation on it? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants