Chary/multisig sign update #1464
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce several enhancements to the transaction handling in the server's handler and schema components. Key modifications include the addition of a Changes
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…te into chary/multisig-sign-update
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
server/schema/update_schema.sql (1)
24-25: Consider adding an index if signed_at will be frequently queried.If this column will be used in WHERE clauses or ORDER BY statements (e.g., filtering or sorting by signing time), consider adding an index to improve query performance.
Example index creation:
CREATE INDEX IF NOT EXISTS idx_transactions_signed_at ON transactions(signed_at);server/handler/transactions.go (4)
148-153: Refactor: Handle NULL 'signed_at' usingsql.NullTimeCurrently, you're using
COALESCEin your SQL queries to replaceNULLsigned_atvalues with a default timestamp'0001-01-01 00:00:00'. This can be confusing and may lead to incorrect time comparisons. Instead, consider allowingsigned_atto beNULLin your queries and handle it usingsql.NullTimein Go. This approach is more idiomatic and makes the nullability explicit.Apply the following changes:
- rows, err = h.DB.Query(`SELECT t.id, COALESCE(t.signed_at, '0001-01-01 00:00:00'::timestamp) AS signed_at, t.multisig_address, t.status, ...`, ...) + rows, err = h.DB.Query(`SELECT t.id, t.signed_at, t.multisig_address, t.status, ...`, ...)In your Go code:
- var signedAt time.Time + var signedAt sql.NullTimeAnd adjust the assignment:
- if signedAt.IsZero() { - transaction.SignedAt = time.Time{} // Set it to zero time if not set - } else { - transaction.SignedAt = signedAt // Otherwise, set the actual signed time - } + if signedAt.Valid { + transaction.SignedAt = signedAt.Time + } else { + // Handle NULL value as appropriate, e.g., leave as zero value or set to `nil` if using pointer + }
Line range hint
176-206: SimplifysignedAthandling withsql.NullTimeBy using
sql.NullTimeforsignedAt, you can eliminate the manual checks for zero values and directly assign the timestamp if it's valid. This enhances code readability and reduces potential errors.After updating
signedAttosql.NullTime, adjust the assignment as shown above.
249-253: Consistent handling ofsigned_atinGetAllMultisigTxnsEnsure that you apply the same
sql.NullTimehandling in theGetAllMultisigTxnsmethod. Remove theCOALESCEfrom your SQL queries and handleNULLvalues in Go.Modify your SQL queries:
- rows, err = h.DB.Query(`SELECT t.id, COALESCE(t.signed_at, '0001-01-01 00:00:00'::timestamp) AS signed_at, t.multisig_address, t.status, ...`, ...) + rows, err = h.DB.Query(`SELECT t.id, t.signed_at, t.multisig_address, t.status, ...`, ...)And use
sql.NullTimeas previously described.
Line range hint
275-303: Applysql.NullTimehandling in transaction scanningIn the loop where you scan transaction data, update the
signedAtvariable to usesql.NullTimeand adjust the assignment accordingly. This maintains consistency across your codebase.Adjust your variable declaration and assignment:
- var signedAt time.Time + var signedAt sql.NullTimeAnd after scanning:
+ if signedAt.Valid { + transaction.SignedAt = signedAt.Time + } else { + // Handle NULL value appropriately + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
server/handler/transactions.go(8 hunks)server/schema/transactions.go(2 hunks)server/schema/update_schema.sql(1 hunks)
🔇 Additional comments (4)
server/schema/update_schema.sql (1)
16-27: LGTM! Safe and consistent schema update implementation.
The implementation follows PostgreSQL best practices by:
- Using IF NOT EXISTS to prevent errors on repeated runs
- Following the same pattern as existing code
- Using appropriate data type for timestamp tracking
server/schema/transactions.go (1)
20-20: LGTM! Field addition looks good.
The new SignedAt field in the Transaction struct is well-structured with appropriate tags and consistent with the existing timestamp fields.
server/handler/transactions.go (2)
457-457: Ensure consistent timezone usage when updating signed_at
When updating the signed_at field in the database, you're using time.Now().UTC(). Confirm that all timestamps across your application are stored and compared using UTC to prevent timezone-related bugs.
To verify consistency, you can check other instances where timestamps are set or compared.
552-562: Verify the logic for clearing signatures after deletion
Ensure that the logic to clear signatures for transactions with a signed_at greater than the deleted transaction's signed_at aligns with the intended business rules. There might be edge cases where transactions have the same signed_at timestamp or where signed_at is NULL.
Consider adding tests to cover these scenarios or reviewing the transaction flow to confirm correctness.
Summary by CodeRabbit
New Features
signed_attimestamp to track when transactions are signed.Bug Fixes
Documentation