-
Notifications
You must be signed in to change notification settings - Fork 171
Add transaction depth APIs for tracking L1 inclusion finality #2749
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
base: main
Are you sure you want to change the base?
Conversation
| }; | ||
|
|
||
| // Track the block height at which this transaction was included | ||
| const inclusionBlockHeight = Number(networkState.blockchainLength.toBigint()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LocalBlockchain has no notion of slots, blocks etc so this will always be 1, throwing off the user. For now, it should be fine to hardcode this at a specific value (or keep using this one) but make sure that we account for this static slot behaviour in getDepth safeGetDepth .. functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true! But I think it would help testing the advancements of slots with local blockchain as described in the issue about LocalBlockchain Support.
const Local = await Mina.LocalBlockchain({ proofsEnabled: false });
Mina.setActiveInstance(Local);
// ... send transaction ...
const included = await pending.wait();
// Initially depth is 0
let info = await included.getDepth({ finalityThreshold: 3 });
console.log(info.depth); // 0
// Simulate blocks being added
Local.setBlockchainLength(Local.getNetworkState().blockchainLength.add(3));
// Now depth is 3
info = await included.getDepth({ finalityThreshold: 3 });
console.log(info.depth); // 3
console.log(info.isFinalized); // true| const finalityThreshold = options?.finalityThreshold ?? DEFAULT_FINALITY_THRESHOLD; | ||
| const currentBlockHeight = getCurrentBlockHeight(); | ||
| // Depth should never be negative (safeguard against edge cases) | ||
| const depth = Math.max(0, currentBlockHeight - inclusionBlockHeight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment re slot behaviour above
src/lib/mina/v1/transaction.ts
Outdated
| ): Promise<TransactionDepthInfo> => { | ||
| const finalityThreshold = options?.finalityThreshold ?? 15; | ||
| const interval = options?.interval ?? 60000; | ||
| const maxAttempts = options?.maxAttempts ?? 30; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the interval being 60s, max attempts 30 and the expected finality threshold 15 we should expect a transaction to be final after at least 60min, likely even more - right now this stops any attempts after 30 * 60s = 30mins, so we already know this will never succeed because the math doesn't work out. we should at least set up the default parameters within a range where we expect the transaction to be final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually requested by @rpanic saying that even though probably nobody would await that promise in the main thread, having this method would remove a lot of complexity from the applications side .i.e for UIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I think having this function is good but right now it's guaranteed to fail because the attempts don't match the default finality. What i am saying is we should change the default parameters to at least have a realistic chance of succeeding! right now the default parameters guarantee that the function will fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! I chose those default params to await for the highest probability for a canonical finality; also taking into consideration the network instability. Btw, even these params, with lower threshold and higher interval & attempts failed when testing Devnet.
What do you recommend setting as default params here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the default threshold is 15 blocks and we expect a block to be around 4.5 on mainnet then that's at least 4.5min * 15 = 67.5min for the function to be successful so we should aim for something that at least attempts the request for 70min; i would change interval to 2min and attempts to 45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! roger that 🫡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the defaults here: 5c5f029
Closes #2729.