Overledger New actions added (read from smart contract and sign a transaction) and modification of existing Actions (Prepare and execute transactions) - tied to issue ticket submitted to Pipedream #14228
Conversation
…from-smart-contract.mjs Success message alteration to align with current context Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…from-smart-contract.mjs inputParameters is an array of Objects so no need to parseObject Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WalkthroughThis pull request introduces several enhancements to the Overledger components, including updates to property descriptions, the addition of new properties, and the introduction of new modules for reading and signing smart contract transactions. Key modifications include the addition of an Changes
Possibly related issues
Possibly related PRs
Suggested labels
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
|
@philbuuza is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
components/overledger/actions/execute-signed-transaction/execute-signed-transaction.mjs (3)
14-15: Approved: Improved clarity inrequestIddescription.The updated description for the
requestIdproperty provides more specific guidance to users, which aligns well with the PR objectives. This change will help ensure that users correctly link this action with the 'Prepare a Smart Contract Transaction' action.Consider adding a note about the importance of using the correct
requestIdto maintain the transaction chain integrity.
19-20: Approved: Enhanced clarity insignedTransactiondescription.The updated description for the
signedTransactionproperty provides clearer guidance on the expected input, which aligns well with the PR objectives. This change will help users correctly populate this field with the output from the 'Sign a Transaction' action.Consider adding a brief note about the importance of not modifying the signed transaction to maintain its validity.
Line range hint
24-34: Consider enhancing error handling and logging in therunmethod.While the current changes improve the action's usability through better property descriptions, there's an opportunity to further enhance the
runmethod:
- Add more detailed error handling to provide specific error messages for different failure scenarios.
- Implement more comprehensive logging to aid in debugging and monitoring.
- Consider adding input validation for the
requestIdandsignedTransactionproperties before making the API call.These improvements would make the action more robust and easier to troubleshoot.
Would you like assistance in implementing these enhancements?
components/overledger/common/constants.mjs (1)
53-60: LGTM! Consider standardizing comment style.The addition of the
UNIT_OPTIONSconstant is well-implemented and consistent with the existing code. It provides a clear mapping between technology options and their respective token symbols, which will be useful for other parts of the application.Consider standardizing the comment style for consistency:
export const UNIT_OPTIONS = { - "ethereum": "ETH", // Ethereum's token symbol is ETH - "substrate": "DOT", // Polkadot's token symbol is DOT - "xrp ledger": "XRP", // XRP Ledger's token symbol is XRP - "bitcoin": "BTC", // Bitcoin's token symbol is BTC - "hyperledger fabric": "FAB", // Placeholder for Hyperledger Fabric's token symbol + "ethereum": "ETH", // Token symbol for Ethereum + "substrate": "DOT", // Token symbol for Polkadot + "xrp ledger": "XRP", // Token symbol for XRP Ledger + "bitcoin": "BTC", // Token symbol for Bitcoin + "hyperledger fabric": "FAB", // Placeholder token symbol for Hyperledger Fabric };This change makes the comments more concise and uniform.
components/overledger/actions/prepare-smart-contract-transaction/prepare-smart-contract-transaction.mjs (3)
27-31: LGTM: NewsmartContractIdproperty addedThe addition of the
smartContractIdproperty is a valuable enhancement, allowing users to specify the smart contract they want to interact with. This aligns well with the PR objectives.Consider adding a
required: truefield to this property definition, as it seems to be a crucial parameter for the action. This would ensure that users always provide this necessary information.
58-67: LGTM: Improved request body structureThe introduction of the
requestBodyobject is a good refactoring step. It consolidates all necessary parameters for the transaction preparation, improving code organization and readability. The inclusion of the newsmartContractIdand the direct processing ofinputParametersusingparseObjectare appropriate changes.Consider adding a comment above the
requestBodydeclaration to briefly explain its purpose and structure. This would further enhance code maintainability.
Line range hint
1-77: Overall assessment: Well-structured and aligned with PR objectivesThe changes made to this file significantly improve the functionality and clarity of the Overledger app's smart contract transaction preparation process. The addition of the
smartContractIdproperty, the restructuring of the request body, and the updates to property descriptions all contribute to a more robust and user-friendly implementation.The modifications align well with the PR objectives of enhancing the Overledger app within Pipedream by improving the prepare action for smart contract transactions.
To further improve the code:
- Consider adding input validation for the
smartContractIdand other critical parameters to ensure data integrity before making the API call.- If not already implemented elsewhere, consider adding error handling for the API call to provide meaningful feedback to the user in case of failures.
components/overledger/actions/read-from-a-smart-contract/read-from-smart-contract.mjs (1)
46-53: EnsurelocationNetworkis always added topropsIn the
additionalPropsmethod, ifthis.locationTechnologyis not defined,locationNetworkwon't be added toprops, which could lead to issues during runtime.Consider making
locationTechnologya required field or providing a default value to ensurelocationNetworkis consistently added.components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs (1)
19-19: Ensure consistent labeling for form fieldsThe
labelfor therequestIdproperty is"requestId", which is inconsistent with the title case used in other labels such as"Signing Account ID". For consistency and better user experience, consider changing it to"Request ID".Apply this diff to fix the label:
label: "requestId", + label: "Request ID",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- components/overledger/actions/execute-signed-transaction/execute-signed-transaction.mjs (1 hunks)
- components/overledger/actions/prepare-smart-contract-transaction/prepare-smart-contract-transaction.mjs (2 hunks)
- components/overledger/actions/read-from-a-smart-contract/read-from-smart-contract.mjs (1 hunks)
- components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs (1 hunks)
- components/overledger/common/constants.mjs (1 hunks)
- components/overledger/overledger.app.mjs (4 hunks)
- components/overledger/sources/watch-new-account-event-instant/watch-new-account-event-instant.mjs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/overledger/sources/watch-new-account-event-instant/watch-new-account-event-instant.mjs
🧰 Additional context used
📓 Learnings (1)
components/overledger/actions/read-from-a-smart-contract/read-from-smart-contract.mjs (2)
Learnt from: lukerQuant PR: PipedreamHQ/pipedream#14217 File: components/overledger/actions/read-from-a-smart-contract/read-from-smart-contract.mjs:27-32 Timestamp: 2024-10-07T08:20:28.836Z Learning: In Overledger actions, define `inputParameters` and `outputParameters` as object arrays, and ensure they are treated consistently throughout the code. Refer to existing actions for examples.Learnt from: lukerQuant PR: PipedreamHQ/pipedream#14217 File: components/overledger/actions/read-from-a-smart-contract/read-from-smart-contract.mjs:57-59 Timestamp: 2024-10-07T08:21:22.593Z Learning: In Overledger actions, throw an error in the `run` method if `locationNetwork` is not selected, as not all Overledger customers have access to the same network.
🔇 Additional comments (7)
components/overledger/actions/execute-signed-transaction/execute-signed-transaction.mjs (1)
Line range hint
1-34: Overall assessment: Positive improvements with room for further enhancements.The changes made to this file successfully improve the clarity and usability of the "Execute Signed Transaction" action. The updated property descriptions provide more specific guidance to users, which aligns well with the PR objectives of enhancing the Overledger app's functionality.
While these changes are valuable, there are opportunities for further improvements:
- Enhancing error handling and logging in the
runmethod.- Adding input validation for the properties.
- Providing additional context in the property descriptions about the importance of using correct values.
These suggested enhancements would further improve the robustness and user-friendliness of the action.
components/overledger/actions/prepare-smart-contract-transaction/prepare-smart-contract-transaction.mjs (2)
25-26: LGTM: Improved property descriptionThe updated description for
signingAccountIdis more concise and accurately describes its purpose. This change enhances the clarity of the code.
70-71: LGTM: Updated method callThe update to the
prepareSmartContractTransactionmethod call correctly utilizes the newly createdrequestBodyobject. This change is consistent with the previous modifications and enhances the overall code clarity and structure.components/overledger/overledger.app.mjs (4)
17-20: LGTM: Addition of the_sandboxBaseUrlmethodThe
_sandboxBaseUrlmethod is correctly implemented to provide the sandbox base URL.
40-43: Verify the use of sandbox base URL inprepareSmartContractTransactionThe
prepareSmartContractTransactionmethod now usesthis._sandboxBaseUrl(). Please confirm that this method should always interact with the sandbox environment. If production usage is also intended, consider making the base URL configurable.
53-60: LGTM: Addition of thesignTransactionmethodThe
signTransactionmethod is correctly implemented using the sandbox base URL.
64-67: Verify the use of sandbox base URL inexecuteSignedTransactionThe
executeSignedTransactionmethod now usesthis._sandboxBaseUrl(). Please confirm that this change is intentional and that this method should interact with the sandbox environment.
components/overledger/actions/read-from-a-smart-contract/read-from-smart-contract.mjs
Outdated
Show resolved
Hide resolved
components/overledger/actions/read-from-a-smart-contract/read-from-smart-contract.mjs
Outdated
Show resolved
Hide resolved
components/overledger/actions/read-from-a-smart-contract/read-from-smart-contract.mjs
Outdated
Show resolved
Hide resolved
components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hi @philbuuza, Thanks for your contribution! LGTM! Ready for QA!
|
@luancazarine just did one little additional update to the overledger.app which will allow for instance selection between Sandox version and Live Overledger as they use different BaseUrls. once user selects an option in the props dropdown menu in environment prop this will set the correct baseURLs in the methods. |
|
Please if you can approve after reviewing this so I can test it in the UI. Many thanks |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/overledger/overledger.app.mjs (1)
6-6: Correct the typos in the comment.In the comment on line 6, there are typos:
- "Sanbox" should be "Sandbox".
- "ro determine" should be "to determine".
… Live Overledger - altered basURL in hook methods create and delete
|
Hi @philbuuza, I'm trying to push a commit with some adjusts, but it looks like you changed some permissions. Could you please check? |
|
Hi @luancazarine. I think it is to do with the versioning. I did not increment the versioning on the components i altered so my mistake as well as folder naming structure I will fix these now. Thanks. |
…ll as component key adjustments to alin with pipedream requirements
|
have ammended the Sign-a-Transaction failed issue with the nativeData: "Sign A Transaction - Fail Have added a default empty object to fix this undefined issue. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs (3)
18-24: Enhance the description and error handling for locationTechnologyThe description for the
locationTechnologyprop could be more specific about its purpose and accepted values. Additionally, consider adding error handling for cases where the previous step doesn't provide a value.Consider updating the prop as follows:
locationTechnology: { type: "string", label: "Location Technology", description: "The blockchain technology used for this transaction (e.g., 'ethereum', 'substrate'). This is required to set the correct dltfee unit.", default: ({ steps }) => { const prevValue = steps.prepare_smart_contract_transaction?.locationTechnology; if (!prevValue) { console.warn("No locationTechnology provided from previous step. Defaulting to 'ethereum'."); } return prevValue || "ethereum"; }, },
40-46: Improve error handling for nativeData default valueThe
nativeDataprop's default value relies on the output of a previous step. It's good practice to add some error handling to ensure the prop receives valid data.Consider updating the prop as follows:
nativeData: { type: "object", label: "Native Data", description: "A JSON object representing the transaction required to be signed.", default: ({ steps }) => { const prevData = steps.prepare_smart_contract_transaction?.nativeData; if (!prevData || Object.keys(prevData).length === 0) { throw new Error("No valid nativeData provided from the previous step."); } return prevData; }, },This change will throw an error if the previous step doesn't provide valid
nativeData, preventing potential issues later in the execution.
74-75: Enhance the summary messageThe current summary message is static and doesn't provide any specific information about the signed transaction.
Consider including more details in the summary message:
$.export("$summary", `Transaction signed successfully. Request ID: ${this.requestId}`);This change provides more context about the specific transaction that was signed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs (2)
1-2: LGTM: Imports are appropriateThe imports for the Overledger app and UNIT_OPTIONS constant are correctly included and necessary for the module's functionality.
4-9: LGTM: Metadata is well-definedThe metadata for the action, including key, name, description, version, and type, is appropriately defined and aligns with the purpose of signing a transaction using Overledger.
| const gatewayFee = { | ||
| amount: "0", | ||
| unit: "QNT", | ||
| }; | ||
| // Define DLT Fee and dynamically set the 'unit/symbol' from UNIT_OPTIONS | ||
| const dltFee = { | ||
| amount: "0.000019897764079968", | ||
| unit: UNIT_OPTIONS[this.locationTechnology] || "ETH", // Use default if not found | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making fee amounts configurable
The gatewayFee and dltFee amounts are currently hardcoded. This may limit flexibility, especially if fee structures change or differ across networks and environments.
Consider adding these as configurable props:
props: {
// ... existing props
gatewayFeeAmount: {
type: "string",
label: "Gateway Fee Amount",
description: "The amount of the gateway fee",
default: "0",
},
dltFeeAmount: {
type: "string",
label: "DLT Fee Amount",
description: "The amount of the DLT fee",
default: "0.000019897764079968",
},
},Then update the run method to use these props:
const gatewayFee = {
amount: this.gatewayFeeAmount,
unit: "QNT",
};
const dltFee = {
amount: this.dltFeeAmount,
unit: UNIT_OPTIONS[this.locationTechnology] || "ETH",
};This change allows users to specify custom fee amounts when needed.
| const response = await this.overledger.signTransaction({ | ||
| $, | ||
| environment: this.environment, | ||
| data: requestBody, | ||
| }); |
There was a problem hiding this comment.
Add error handling for signTransaction call
The current implementation doesn't handle potential errors from the signTransaction method. It's important to catch and handle these errors gracefully.
Wrap the API call in a try-catch block:
try {
const response = await this.overledger.signTransaction({
$,
environment: this.environment,
data: requestBody,
});
$.export("$summary", "Transaction signed successfully");
return response;
} catch (error) {
console.error("Error signing transaction:", error);
throw error;
}This change will log any errors and re-throw them, allowing for better error handling and debugging.
|
@luancazarine and @vunguyenhung I am still waiting on this PR. Can I get an update as to the progress of this. I dont have an upgraded account which would have allowed me to test this out myself so I kindly ask this is done so I can complete this task. Many thanks. |
|
Hi @philbuuza, sorry for the late response. I've move forward this ticket to PR review. We recommend to test your action to save back-and-forth time if possible. Thank you for your contribution! |
|
Hi @vunguyenhung @luancazarine How do I test my action without being able to use the UI with it. Please if you could help me with this or direct me on how. would be much appreciated |
|
Hi @philbuuza, you can publish your developed action into your own Pipedream account to test using Pipedream CLI. Here's the quick start: https://pipedream.com/docs/components/actions-quickstart |
components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs
Outdated
Show resolved
Hide resolved
components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs
Outdated
Show resolved
Hide resolved
|
@luancazarine any updates on this. Can it be merged soon? |
Hi @lukerQuant. Were you able to test the components as @vunguyenhung suggested? |
Hi @luancazarine , yes @philbuuza tested via the cli. We now look forward to @vunguyenhung's approval. Best wishes |
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
|
Hi @philbuuza, @lukerQuant, as there're some dificulty in using data for testing, would you mind providing test data for each of the application, or could you try to publish the action, test it on your side and share the screenshot here? There's a quick start here simply guide you how to publish your own action |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/overledger/actions/prepare-smart-contract-transaction/prepare-smart-contract-transaction.mjs (2)
33-37: Remove unnecessary newline in smartContractId property definition.The property definition contains an extra newline that affects readability.
smartContractId: { type: "string", label: "Smart Contract ID", - description: "The ID/address of the smart contract to interact with.", - + description: "The ID/address of the smart contract to interact with." },
64-73: Improve inline comment clarity.The comment about parseObject could be more concise and professional.
- inputParameters: parseObject(this.inputParameters), //parse these values using the parseObject function at this shouls turn the JSON string into JSON objects to used in the request body. + inputParameters: parseObject(this.inputParameters), // Convert JSON string parameters to objects for API request
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- components/overledger/actions/prepare-smart-contract-transaction/prepare-smart-contract-transaction.mjs (3 hunks)
- components/overledger/actions/read-from-a-smart-contract/read-from-a-smart-contract.mjs (1 hunks)
- components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/overledger/actions/read-from-a-smart-contract/read-from-a-smart-contract.mjs
- components/overledger/actions/sign-a-transaction/sign-a-transaction.mjs
🧰 Additional context used
🔇 Additional comments (3)
components/overledger/actions/prepare-smart-contract-transaction/prepare-smart-contract-transaction.mjs (3)
11-11: LGTM! Version bump is appropriate.The version increment from 0.0.1 to 0.0.2 aligns with the changes made to the component.
15-20: Environment property implementation looks good.The environment property has been added as requested in the previous review.
74-78: 🛠️ Refactor suggestionConsider adding error handling for API responses.
While the API call is structured correctly, consider adding try-catch blocks to handle potential API errors gracefully.
+ try { const response = await this.overledger.prepareSmartContractTransaction({ $, environment: this.environment, data: requestBody, }); + } catch (error) { + throw new Error(`Failed to prepare smart contract transaction: ${error.message}`); + }
|
Hi @luancazarine and @vunguyenhung, @lukerQuant - Thanks for your time with this PR - as there has been alot of back and forth. All issues have now been sorted and fully tested. The only issue we can not seem to resolve but is not a real issue is read function calls on smart contract functions that do not give an output as @vunguyenhung had discovered on his testing. Have tried all types of solutions including even omitting this parameter altogether on the API request but nothing works as it still throws an "cant find smart contract error' amongst otehr errors. So we can just settle on this not being an issue as no one will really invoke read functions that do not return anything. This should not be a blocker in passing the action. The outputParameter has been made optional and now has a default value of an empty array [] for now. The same has been done for input parameters which is now also an empty array as this always has to be sent in the API call. All other actions including sign transaction have passed. Please find attached some screenshots from testing my published actions from this repo via the CLI. **(read from contract) -b read from contract - c read from contract - d read from contract - e prepare a Tranx Sign a tranx Execute a Tranx |
|
Hi @philbuuza, thank you for your throughout testing. I think it seems good to release this now. Will move to ready for release. |
|
Hi everyone, all test cases are passed! Ready for release! |
|
Hi @luancazarine. If we can get this merge done that would be great. @vunguyenhung has given the ok on the test. |
|
Hi @philbuuza @lukerQuant! It's done! I've already merged it to master!!! |
|
@luancazarine. Great stuff. many thanks for your input on this. Much appreciated |
The Overledger Pipedream Actions did not have the correct properties in order to work with the the Quant Overledger API request parameters. As well as this other actions added to complete the API sequence. We want the Overledger app in pipedream to be able to perform the the read, monitoring and writing actions. please have a look at the issue submitted:
#14150.
Modifications have been to the prepare and execute actions as these did it seem to be functional.
Phil Buuza
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Version Updates