feat: Create escrow_contracts table for escrow contract tracking #143
Conversation
|
@Jagadeeshftw is attempting to deploy a commit to the kindfi Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughA new SQL migration file is introduced that creates an ENUM type, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
services/supabase/migrations/20250209100653_create_escrow_contract_table.sql(1 hunks)
🔇 Additional comments (1)
services/supabase/migrations/20250209100653_create_escrow_contract_table.sql (1)
1-2: Well-defined ENUM creation for escrow_state
You’ve clearly defined a custom ENUM type for escrow_state with the essential states. Consider adding a brief comment (or documentation) to explain the purpose of each status (i.e., when to use 'NEW', 'ACTIVE', and 'CANCELLED') for future clarity.
|
@AndlerRL Where I need to update EscrowContract TypeScript definitions can you add more info? |
@Jagadeeshftw there is already indications in the PD: Did you add the RLS policies? I don't see them in the changes. Additionally, I see that your commits are not signed, please sign them to adhere to our |
Jagadeeshftw
force-pushed
the
escrow-contract
branch
from
797af0c to
0d97fdc
Compare
coderabbitai
bot
added
documentation
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
services/supabase/migrations/20250209100653_create_escrow_contract_table.sql(1 hunks)
🔇 Additional comments (6)
services/supabase/migrations/20250209100653_create_escrow_contract_table.sql (6)
1-2: Enum Creation is Clean.
Theescrow_stateenum is defined clearly with the expected values. Consider adding a brief inline comment about its purpose to help future maintainers understand when and how to extend it if needed.
5-6: Unique Identifiers Defined.
Theengagement_idandcontract_idfields are properly set as UNIQUE and NOT NULL, which helps enforce the uniqueness of these domain-specific identifiers.
7-8: Foreign Keys and Referential Integrity.
Theproject_idandcontribution_idcolumns correctly reference their respective tables. It’s a good idea to double-check that the referenced columns in theprojectsandcontributionstables use UUIDs to ensure type consistency.
11-12: Amount and State Configuration.
Theamountfield is defined with sufficient precision and is later validated by the check constraint (ensuring it is positive). Thecurrent_statecolumn leverages theescrow_stateenum with a default of 'NEW', aligning well with expected behavior.
17-17: Metadata Field Implementation.
Themetadatafield, defined as JSONB with a default of an empty JSON object, is a good choice for future extensibility and flexible data storage.
18-18: Amount Check Constraint.
The check constraint onamount(ensuring it is greater than 0) effectively reinforces data integrity for escrow amounts.
| CREATE TYPE IF NOT EXISTS escrow_state AS ENUM ('NEW','ACTIVE','CANCELLED'); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS escrow_contracts ( | ||
| id UUID PRIMARY KEY DEFAULT uuid_generate_v4(), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
UUID Generation Dependency.
The primary key uses uuid_generate_v4(), which is great for ensuring unique identifiers. However, please ensure that the PostgreSQL extension (typically uuid-ossp) is enabled in your database setup or a prior migration to avoid runtime issues.
| payer_address TEXT NOT NULL, | ||
| receiver_address TEXT NOT NULL, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Address Fields Verification.
The payer_address and receiver_address fields are defined as TEXT and are mandated as NOT NULL. If there are business rules regarding the format or maximum length of these addresses, consider implementing those validations at the application layer or via an additional check constraint.
| receiver_address TEXT NOT NULL, | ||
| amount NUMERIC(20,7) NOT NULL, | ||
| current_state escrow_state NOT NULL DEFAULT 'NEW', | ||
| platform_fee NUMERIC(5,2) NOT NULL, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Platform Fee Constraint Suggestion.
The platform_fee column is defined with a fixed precision, which is appropriate; however, it currently lacks a check constraint to ensure a non-negative value (or to enforce any business-specific upper bounds). Consider adding a constraint if such validations are required.
| created_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Timestamp Columns Reminder.
Both the created_at and updated_at fields default to CURRENT_TIMESTAMP, which is useful for record creation. Do note that the updated_at column will not automatically update on record modifications. If automatic timestamp updates upon changes are desired, consider implementing a trigger or handling this update within the application logic.
| CREATE TABLE IF NOT EXISTS escrow_contracts ( | ||
| id UUID PRIMARY KEY DEFAULT uuid_generate_v4(), | ||
| engagement_id TEXT UNIQUE NOT NULL, | ||
| contract_id TEXT UNIQUE NOT NULL, | ||
| project_id UUID NOT NULL REFERENCES projects(id), | ||
| contribution_id UUID NOT NULL REFERENCES contributions(id), | ||
| payer_address TEXT NOT NULL, | ||
| receiver_address TEXT NOT NULL, | ||
| amount NUMERIC(20,7) NOT NULL, | ||
| current_state escrow_state NOT NULL DEFAULT 'NEW', | ||
| platform_fee NUMERIC(5,2) NOT NULL, | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, | ||
| completed_at TIMESTAMP WITH TIME ZONE, | ||
| metadata JSONB DEFAULT '{}'::jsonb, | ||
| CONSTRAINT valid_escrow_amount CHECK (amount > 0) | ||
| ); |
There was a problem hiding this comment.
Row Level Security (RLS) Policies Missing.
It appears that no Row Level Security (RLS) policies are applied to the escrow_contracts table. If this table contains sensitive contract details, consider adding RLS policies in this migration or in a subsequent one to enforce fine-grained access control.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
services/supabase/migrations/20250209100653_create_escrow_contract_table.sql (1)
3-20: Table Schema and Field Constraints Review.
Theescrow_contractstable is structured with thoughtful constraints and data types:
- The primary key on
idusesuuid_generate_v4(); as mentioned in previous reviews, please ensure that the necessary PostgreSQL extension (uuid-ossp) is enabled.- Unique constraints on
engagement_idandcontract_idhelp maintain data integrity; if the business logic prescribes a specific format for these identifiers, consider adding application-level validations.- Foreign key references on
project_idandcontribution_idare correctly defined; review whether cascading actions (e.g.,ON DELETE CASCADE) are desired based on relational consistency rules.- The address fields (
payer_addressandreceiver_address) are marked as NOT NULL. Similar to past feedback, if there are specific format or length requirements, adding a check constraint or handling these validations at the application layer might be beneficial.- Numeric fields such as
amountandplatform_feeinclude check constraints (ensuringamount > 0andplatform_fee >= 0), which is excellent for data integrity.- Timestamp fields use
CURRENT_TIMESTAMPas default; note that whilecreated_atis auto-populated at creation,updated_atwill not update on subsequent modifications unless managed by a trigger or application logic. Consider implementing an automatic update mechanism if that behavior is desired.
Overall, the table design aligns well with the migration objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
services/supabase/migrations/20250209100653_create_escrow_contract_table.sql(1 hunks)
services/supabase/migrations/20250209100653_create_escrow_contract_table.sql
Show resolved
Hide resolved
| -- Enable Row Level Security (RLS) on the table | ||
| ALTER TABLE escrow_contracts ENABLE ROW LEVEL SECURITY; | ||
|
|
||
| -- RLS Policy: allow read access to all | ||
| CREATE POLICY public_select_escrow_contracts | ||
| ON escrow_contracts | ||
| FOR SELECT | ||
| USING (true); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Row Level Security (RLS) and Policy Configuration.
Enabling RLS on the escrow_contracts table and defining a public SELECT policy is a strong move towards enhancing the security posture of the data. Consider reviewing future needs to determine if additional policies—for INSERT, UPDATE, or DELETE operations—are required as the business logic evolves.
|
@AndlerRL Could you please review the PR when you get a chance? Let me know if any changes are needed. Thanks! |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Thanks for signing your commits 🙏 I see the SQL looks good, I noticed that supabase CI/CD did not triggered (was sleeping) so I am not marking them as resolved the coderabbitai observations yet until it triggers, and also because I found that there are no new TS definitions within the services/supabase folder. Make sure to follow step 3 from the related readme. After that and triggering successfull the supabase CI/CD (when changes has been made within the folder), we are good to go!
|
Hey @Jagadeeshftw checking the PR I noticed this is a fork merge. According to our How to Contribute guide from our development guides, we have to create a branch based on the Also remember the rest of the observations. Thanks! 🙏 |
This MR introduces a new SQL migration that creates the escrow_contracts table along with the escrow_state enum.
The new table is intended to store escrow contract details for the off-chain track of project contributions and time status management
Closes #91
Summary by CodeRabbit