Skip to content

Migrate db write to atomic - #51

Open
Bastia01 wants to merge 16 commits into
developfrom
migrate-db-write-to-atomic
Open

Migrate db write to atomic#51
Bastia01 wants to merge 16 commits into
developfrom
migrate-db-write-to-atomic

Conversation

@Bastia01

Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request migrates database write operations to use atomic transactions, significantly improving data consistency and reliability across the application. The changes introduce a new transaction management pattern using GORM's transaction support, along with several important bug fixes and security improvements.

Changes:

  • Introduced WithTransaction helper and getExecutor function to enable optional transaction support in storage layer functions
  • Migrated storage write functions to accept *gorm.DB parameter, allowing them to participate in transactions
  • Wrapped related database operations in service layer with WithTransaction to ensure atomicity
  • Fixed SQL injection vulnerability, array bounds checking issues, signal handling, and various error handling bugs
  • Changed GetOrCreateAccount calls to GetAccount with explicit nil checks, preventing unintended account creation

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.

Show a summary per file
File Description
storage/tx.go New file introducing WithTransaction wrapper and getExecutor helper for transaction management
storage/userInfoStorer.go Migrated CreateUserInfo to accept transaction parameter
storage/statsStorer.go Migrated CreateStats to accept transaction parameter
storage/preferencesStorer.go Migrated preference functions to support transactions; added GetPreferenceByAddressForUpdate with SELECT FOR UPDATE locking
storage/kycStorer.go Migrated CreateOrUpdateKyc to accept transaction; improved error handling with errors.Is
storage/invoiceStorer.go Fixed SQL injection vulnerability and error handling bug
storage/invoiceDraftStorer.go Migrated invoice draft functions to accept transaction parameter
storage/burnEventStorer.go Migrated CreateBurnEvent to accept transaction parameter
storage/allocationStorer.go Migrated allocation functions to accept transaction parameter
storage/accountStorer.go Migrated UpdateAccount to accept transaction parameter
storage/sellerStorer.go Added nil check for GetSellerByCode when no rows found
service/sumsubService.go Wrapped KYC updates in transaction; fixed typo in error message
service/statsService.go Wrapped stats generation in transaction for atomicity
service/oblioService.go Enhanced error handling to include warning in email notification
service/monthlyPoaiInvoiceService.go Refactored to use transactions with SELECT FOR UPDATE locking to prevent race conditions in invoice number generation
service/emailService.go Added optional warning parameter to buy license email
service/accountService.go Added GetAccount function; wrapped account/KYC updates in transactions; reordered operations to send email before DB update (introduces potential issue)
proxy/handlers/sumsub.go Changed to use GetAccount instead of GetOrCreateAccount with explicit nil check
proxy/handlers/seller.go Changed to use GetAccount; added nil checks for empty slices
proxy/handlers/license.go Changed to use GetAccount; moved invoice creation after signature generation
proxy/handlers/invoiceDraft.go Updated calls to pass nil for transaction parameter
proxy/handlers/burnReport.go Added bounds checking to prevent index out of range panics
proxy/handlers/account.go Changed multiple handlers to use GetAccount; reordered operations in blacklist handler (introduces potential issue)
cmd/main.go Fixed signal handling to use syscall.SIGTERM instead of deprecated os.Kill
Comments suppressed due to low confidence (3)

service/accountService.go:20

  • Typo in function name: "getAcocunt" should be "getAccount". This function is called in the newly added GetAccount function.
	account, err := getAcocunt(address)

service/accountService.go:84

  • The order of operations here creates a potential inconsistency. The confirmation email is sent on line 73 before the account is updated on line 81. If the database update fails after the email is sent, the user will receive a confirmation email but the account won't have the pending email recorded, making the confirmation link invalid. Consider moving the database update before sending the email, or wrapping both operations in a transaction with a rollback mechanism for the email.
	err = SendConfirmEmail(address, email)
	if err != nil {
		return nil, errors.New("error while sending confirmation email: " + err.Error())
	}

	account.PendingEmail = email
	account.PendingReceiveUpdates = receiveUpdates

	err = storage.UpdateAccount(nil, account)
	if err != nil {
		return nil, errors.New("error while updating account on storage: " + err.Error())
	}

proxy/handlers/account.go:458

  • The order of operations creates a potential inconsistency. The blacklist email is sent on line 424 before the account is updated on line 453. If the database update fails after the email is sent, the user will receive a blacklist notification but the account won't actually be blacklisted in the database. Consider moving the database update before sending the email, or wrapping both operations in a transaction-like pattern.
		err = service.SendBlacklistedEmail(*account.Email)
		if err != nil {
			log.Error("error while sending blacklisted email: " + err.Error())
			model.JsonResponse(c, http.StatusBadRequest, nil, nodeAddress, err.Error())
			return
		}

		kyc, _, err := storage.GetKycByEmail(*account.Email)
		if err != nil {
			log.Error("error while retrieving kyc information from storage: " + err.Error())
			model.JsonResponse(c, http.StatusInternalServerError, nil, nodeAddress, err.Error())
			return
		}

		accountDto, err = service.NewAccountDto(account, kyc)
		if err != nil {
			log.Error("error while creating account dto: " + err.Error())
			model.JsonResponse(c, http.StatusInternalServerError, nil, nodeAddress, err.Error())
			return
		}
	} else {
		accountDto, err = service.NewAccountDto(account, nil)
		if err != nil {
			log.Error("error while creating account dto: " + err.Error())
			model.JsonResponse(c, http.StatusInternalServerError, nil, nodeAddress, err.Error())
			return
		}
	}

	err = storage.UpdateAccount(nil, account)
	if err != nil {
		log.Error("error while updating account: " + err.Error())
		model.JsonResponse(c, http.StatusInternalServerError, nil, nodeAddress, err.Error())
		return
	}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Bastia01
Bastia01 force-pushed the migrate-db-write-to-atomic branch from fcf869f to c65a997 Compare February 20, 2026 16:44
@aledefra
aledefra force-pushed the migrate-db-write-to-atomic branch from c65a997 to e029bbc Compare June 10, 2026 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants