-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: swap icy webhook #770
base: develop
Are you sure you want to change the base?
Conversation
f9557e2
to
3863046
Compare
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.
PR Review Summary
I've reviewed the changes for implementing the ICY to BTC swap functionality and webhook. The implementation looks promising but there are several items that should be addressed before merging:
Strengths
- Good database schema design with proper migration script
- Clean contract binding implementation
- Configuration changes follow existing patterns
Areas for Improvement
-
Database Indexing: Consider adding indexes to the new
icy_swap_btc_requests
table for columns that will be frequently queried:CREATE INDEX idx_icy_swap_btc_profile_id ON icy_swap_btc_requests(profile_id); CREATE INDEX idx_icy_swap_btc_request_code ON icy_swap_btc_requests(request_code);
-
Configuration Documentation: The new configuration values should be documented:
- What is the expected format for
ICY_BACKEND_PRIVATE_KEY
? - Should we add examples in the README or .env.example file?
- What is the expected format for
-
Naming Consistency: The change from "swap" to "icyswapbtc" in the transaction action type is good for clarity, but ensure this is applied consistently across the entire codebase to prevent runtime errors.
-
Missing Core Implementation: The webhook controller implementation files don't appear in the PR changes. Please ensure all necessary files are included.
-
Testing: No test files are included - recommend adding at least integration tests for the swap workflow.
-
Error Handling: For blockchain operations, especially around token transfers, robust error handling is critical.
-
Security: Since this involves financial transactions:
- Validate all input parameters thoroughly
- Implement proper authentication for webhook endpoints
- Consider rate limiting to prevent abuse
Overall, this is a good start but needs additional work before it's ready to merge. Please address these items and feel free to reach out if you need clarification on any points.
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'm requesting changes for the following reasons:
-
Database Schema Improvements Needed:
- Consider adding indexes for frequently queried columns such as
profile_id
andrequest_code
to improve query performance:
CREATE INDEX idx_icy_swap_btc_profile_id ON icy_swap_btc_requests(profile_id); CREATE INDEX idx_icy_swap_btc_request_code ON icy_swap_btc_requests(request_code);
- Consider adding indexes for frequently queried columns such as
-
Configuration Documentation Missing:
- The new config values added in
pkg/config/config.go
need documentation about their purpose and format - Consider adding examples to the README or .env.example file
- The new config values added in
-
API Consistency Concerns:
- The change from "swap" to "icyswapbtc" in transaction actions is significant
- Ensure this is applied consistently across the entire codebase to prevent runtime errors
-
Contract Interface Issues:
- In
pkg/contracts/icyswap/icyswap.go
, the function signature comments still reference "swap" while the code calls "icyswapbtc" - These should be updated to match for clarity
- In
-
Missing Implementation:
- The webhook controller implementation files don't appear in the PR changes
- Please ensure all necessary files are included
-
Testing Strategy:
- Add at least integration tests for the swap workflow to ensure reliability
Please address these issues before the PR is ready for merging.
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.
First issue regarding the database schema.
token_name varchar(255), | ||
token_id varchar(255), | ||
swap_request_status varchar(255), | ||
revert_status varchar(255), |
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.
Consider adding indexes for frequently queried columns such as profile_id
and request_code
to improve query performance:
CREATE INDEX idx_icy_swap_btc_profile_id ON icy_swap_btc_requests(profile_id);
CREATE INDEX idx_icy_swap_btc_request_code ON icy_swap_btc_requests(request_code);
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 have several specific issues to address:
-
Database Schema (migrations/schemas/20250320153030-create-icy_btc_swap_request.sql):
Consider adding indexes for frequently queried columns such asprofile_id
andrequest_code
to improve query performance:CREATE INDEX idx_icy_swap_btc_profile_id ON icy_swap_btc_requests(profile_id); CREATE INDEX idx_icy_swap_btc_request_code ON icy_swap_btc_requests(request_code);
-
Configuration (pkg/config/config.go):
Please document the expected format and purpose of these new configuration values for IcyBackend and expanded MochiPay configuration. Also consider adding examples to the README or .env.example file. -
API Consistency (pkg/service/mochipay/request.go):
The change from "swap" to "icyswapbtc" in transaction actions is significant. Ensure this is applied consistently across the entire codebase to prevent runtime errors. -
Contract Documentation (pkg/contracts/icyswap/icyswap.go):
The function signature comments don't match the actual implementation. The comments still reference "swap" while the code calls "icyswapbtc". These should be updated to match for clarity. -
Missing Implementation:
The webhook controller implementation files don't appear in the PR changes. Please ensure all necessary files are included. -
Testing Strategy:
Add integration tests for the swap workflow to ensure reliability.
92b7b6a
to
b1c3431
Compare
b1c3431
to
cee85ee
Compare
What's this PR do?
[wip] swap icy webhook
Flow