-
Notifications
You must be signed in to change notification settings - Fork 40
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
Adding batch timestamp to the public tx API #1740
Conversation
WalkthroughThe system has been updated to track the batch timestamp for transactions. The Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/common/query_types.go (1 hunks)
- go/enclave/storage/enclavedb/batch.go (2 hunks)
Additional comments: 3
go/common/query_types.go (1)
- 36-36: The addition of the
BatchTimestamp
field to thePublicTransaction
struct is consistent with the PR objectives and is correctly typed asuint64
. This change should enhance the transaction metadata as intended.go/enclave/storage/enclavedb/batch.go (2)
47-47: The modification to the
queryTxList
SQL query to includebatch.header
is correct and aligns with the PR objectives to enhance transaction metadata.512-526: The changes to the
selectPublicTxsBySender
function, including the decoding of thebatchHeader
and populating theBatchTimestamp
field in thePublicTransaction
struct, are correctly implemented usingrlp.DecodeBytes
.
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- tools/tenscan/frontend/src/components/modules/dashboard/recent-transactions.tsx (1 hunks)
- tools/tenscan/frontend/src/components/modules/transactions/columns.tsx (1 hunks)
- tools/tenscan/frontend/src/types/interfaces/TransactionInterfaces.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- tools/tenscan/frontend/src/components/modules/dashboard/recent-transactions.tsx
Additional comments: 1
tools/tenscan/frontend/src/types/interfaces/TransactionInterfaces.ts (1)
- 5-5: The addition of the
BatchTimestamp
field to theTransaction
type is correctly implemented and aligns with the PR objectives to provide additional context to transactions.
tools/tenscan/frontend/src/components/modules/transactions/columns.tsx
Outdated
Show resolved
Hide resolved
@Jennievon the timestamp that's displayed doesn't have any treatment (it's a unix timestamp). I wonder if you have some wizard trick to display these nicely ? |
sure, I'll push an update for it. |
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.
LGTM but maybe Jennie can advise on the formatting comment.
@@ -17,6 +17,11 @@ export function RecentTransactions({ transactions }: { transactions: any }) { | |||
#{transaction?.BatchHeight} | |||
</p> | |||
</div> | |||
<div className="ml-4 space-y-1"> | |||
<p className="text-sm font-medium leading-none"> | |||
{transaction?.BatchTimestamp} |
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.
Would it work to use {formatTimeAgo(batch?.timestamp)}
here, like @Jennievon did in recent-batches.tsx
? To get the 'x mins ago' formatting.
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- tools/tenscan/frontend/src/components/modules/dashboard/recent-transactions.tsx (2 hunks)
- tools/tenscan/frontend/src/components/modules/transactions/columns.tsx (2 hunks)
- tools/tenscan/frontend/src/lib/utils.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/tenscan/frontend/src/components/modules/dashboard/recent-transactions.tsx
- tools/tenscan/frontend/src/components/modules/transactions/columns.tsx
export function formatTimeAgo(unixTimestampSeconds: string | number) { | ||
if (!unixTimestampSeconds) { | ||
return "Unknown"; | ||
} |
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.
The update to formatTimeAgo
to accept both string
and number
types for unixTimestampSeconds
adds flexibility. However, ensure that the type conversion with Number()
is safe and won't lead to unexpected behavior with different input types. The addition of a falsy check to return "Unknown" is a good defensive programming practice.
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.
LGTM!
Why this change is needed
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks