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

integrations: Remove extra fetching of runtime public key #379

Closed
wants to merge 1 commit into from

Conversation

aefhm
Copy link
Contributor

@aefhm aefhm commented Sep 4, 2024

Description

Address Node timeout issue encountered by developers.

Im using the alpha of @oasisprotocol/sapphire-viem-v2, and it looks like when using it, node process wont naturally terminate (w/o a call to exit() ). I think that its the fault of this timer

setTimeout(async () => {
await fetcher.fetch(provider);
}, fetcher.timeoutMilliseconds);

Resolution

I don't think we need the second fetch.

@aefhm aefhm self-assigned this Sep 4, 2024
Copy link

netlify bot commented Sep 4, 2024

Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name Link
🔨 Latest commit 0dae328
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/66d8f14619c6290008c6431c

@aefhm aefhm marked this pull request as ready for review September 5, 2024 03:59
@aefhm aefhm requested a review from CedarMist September 5, 2024 03:59
@aefhm aefhm added p:2 Priority: nice to have client javascript Pull requests that update JavaScript code labels Sep 5, 2024
@CedarMist
Copy link
Member

So, this is a workaround for the fact that Viem's code for packing transactions isn't async, meaning when we encrypt a transaction we can't do an RPC request to retrieve the calldata public key.

We do the first fetch to retrieve the public key, and then the timer proactively fetches public keys.

Without the timer and periodic public key fetching the app will eventually start encrypting transactions with expired keys causing it to fail.

It would be good to find a workaround for this, as it's not the best. But just removing it will cause long-running dApps to break.

@CedarMist
Copy link
Member

CedarMist commented Sep 5, 2024

It would be good to setup a test case for using Viem from Node CLI to reproduce.

And use https://www.npmjs.com/package/wtfnode to see which handles are still active.

Could use a WeakRef to allow the callback function to be garbage collected, but that's not 100% predictable, see:

let callback = () => {
  console.log('Interval tick');
  // Your interval logic here
};

const weakCallback = new WeakRef(callback);

const intervalId = setInterval(() => {
  const cb = weakCallback.deref();
  if (cb) {
    cb();
  } else {
    console.log('Callback has been garbage collected');
    clearInterval(intervalId);
  }
}, 1000);

// At some point later
callback = null; // Allow the callback to be garbage collected

Or we could trap SIGINT via process.on to clear the interval... Would need to detect if it's Node or Web via checking if globalThis.process is undefined.

Or we could unref the interval ID, e.g.

https://albraga.gitbooks.io/mastering-node-js-book/content/unref-and-ref.html

The unref method allows the developer to assert the following instructions: when this timer is the only event source remaining for the event loop to process, go ahead and terminate the process.

setTimeout(() => { console.log("now stop"); }, 100); 

let intervalId = setInterval(() => { console.log("running") }, 1); 

intervalId.unref()

@CedarMist
Copy link
Member

Or... we could refactor the Viem code to not do any transaction encryption inside the non-async Viem code, and push it all into the provider code? However, last time I tried that it had trouble as it wasn't always going through the provider and I had trouble intercepting calls to the provider.

@@ -123,9 +123,6 @@ export async function createSapphireSerializer<
const fetcher = new KeyFetcher();
const provider = client as EthereumProvider;
await fetcher.fetch(provider);
setTimeout(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Lets document the reason for having the interval timer.

It be changed to setInterval, so it's continuous.

And the interval ID should be unref'd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. unref seems best right now.

Copy link
Member

Choose a reason for hiding this comment

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

Have you been able to reproduce the problem with a test? Would be good to have this in CI

@matevz
Copy link
Member

matevz commented Sep 10, 2024

Closing this in favor of #383

@matevz matevz closed this Sep 10, 2024
@aefhm aefhm deleted the xz/viem-v2-remove-second-fetch branch September 11, 2024 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client javascript Pull requests that update JavaScript code p:2 Priority: nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants