cron: Added comment for completed RFCs #1
Conversation
|
@coderabbitai review |
|
Warning Rate Limit Exceeded@Bullrich has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 1 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe project has been updated to enhance GitHub actions for notifying on pull requests (PRs) about ongoing and completed referenda. It includes modifications to handle both types of referenda efficiently, involving fetching data through a new Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (3)
- README.md (1 hunks)
- src/cron.ts (7 hunks)
- src/subsquare.ts (1 hunks)
Additional comments: 3
src/subsquare.ts (1)
- 1-1: The import of
node-fetchis correct and necessary for making HTTP requests within the Node.js environment.src/cron.ts (2)
- 50-51: The introduction of
OpenReferendaandCompletedReferendatypes is a good practice for type safety and clarity. These types help in structuring the data related to referenda, making the code more maintainable and understandable.- 148-171: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [128-168]
The modifications to the
cronfunction to handle both ongoing and completed referenda are well-implemented. The logic for matching PRs with referenda and creating comments on PRs is clear and effective. However, ensure that thePROVIDER_URLenvironment variable is correctly configured and used throughout the action's logic, as it's crucial for connecting to the correct blockchain node.Verification successful
The search results confirm that
PROVIDER_URLis correctly configured and used throughout the action's logic, as indicated by its definition insrc/constants.tsand its usage in various parts of the code, such assrc/cron.ts,src/referendum-tx.ts, andsrc/find-referendum-state.ts. This ensures the action connects to the correct blockchain node, aligning with the review comment's concern about the importance ofPROVIDER_URLconfiguration and usage.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify usage and configuration of PROVIDER_URL in the action's logic rg 'PROVIDER_URL' --type tsLength of output: 828
| interface ReferendaObject { | ||
| _id: string; | ||
| referendumIndex: number; | ||
| title: string; | ||
| content: string; | ||
| track: number; | ||
| createdAt: string; | ||
| updatedAt: string; | ||
| lastActivityAt: string; | ||
| state: { | ||
| name: "Executed" | string; | ||
| }; | ||
| onchainData: { | ||
| /** Compare this to tx.method.hash.toHex() */ | ||
| proposalHash: string; | ||
| timeline: { | ||
| _id: string; | ||
| referendumIndex: number; | ||
| name: "Executed" | "Confirmed" | "ConfirmStarted" | "DecisionStarted"; | ||
| indexer: { | ||
| blockHeight: number; | ||
| /** Use this when generating the comment */ | ||
| blockHash: string; | ||
| blockTime: number; | ||
| }; | ||
| }[]; | ||
| }; |
There was a problem hiding this comment.
The ReferendaObject interface is well-defined, covering all necessary properties of a referenda object. However, consider adding documentation comments to each property for better clarity and maintainability.
+ /** Unique identifier of the referenda */
_id: string;
+ /** Index number of the referenda */
referendumIndex: number;
+ /** Title of the referenda */
title: string;
+ /** Content or description of the referenda */
content: string;
+ /** Track number associated with the referenda */
track: number;
+ /** Creation date of the referenda */
createdAt: string;
+ /** Last update date of the referenda */
updatedAt: string;
+ /** Date of the last activity on the referenda */
lastActivityAt: string;
+ /** State of the referenda, with 'Executed' as a possible value */
state: {
name: "Executed" | string;
};
+ /** On-chain data related to the referenda */
onchainData: {
/** Compare this to tx.method.hash.toHex() */
proposalHash: string;
timeline: {
_id: string;
referendumIndex: number;
name: "Executed" | "Confirmed" | "ConfirmStarted" | "DecisionStarted";
indexer: {
blockHeight: number;
/** Use this when generating the comment */
blockHash: string;
blockTime: number;
};
}[];
};Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| interface ReferendaObject { | |
| _id: string; | |
| referendumIndex: number; | |
| title: string; | |
| content: string; | |
| track: number; | |
| createdAt: string; | |
| updatedAt: string; | |
| lastActivityAt: string; | |
| state: { | |
| name: "Executed" | string; | |
| }; | |
| onchainData: { | |
| /** Compare this to tx.method.hash.toHex() */ | |
| proposalHash: string; | |
| timeline: { | |
| _id: string; | |
| referendumIndex: number; | |
| name: "Executed" | "Confirmed" | "ConfirmStarted" | "DecisionStarted"; | |
| indexer: { | |
| blockHeight: number; | |
| /** Use this when generating the comment */ | |
| blockHash: string; | |
| blockTime: number; | |
| }; | |
| }[]; | |
| }; | |
| interface ReferendaObject { | |
| /** Unique identifier of the referenda */ | |
| _id: string; | |
| /** Index number of the referenda */ | |
| referendumIndex: number; | |
| /** Title of the referenda */ | |
| title: string; | |
| /** Content or description of the referenda */ | |
| content: string; | |
| /** Track number associated with the referenda */ | |
| track: number; | |
| /** Creation date of the referenda */ | |
| createdAt: string; | |
| /** Last update date of the referenda */ | |
| updatedAt: string; | |
| /** Date of the last activity on the referenda */ | |
| lastActivityAt: string; | |
| /** State of the referenda, with 'Executed' as a possible value */ | |
| state: { | |
| name: "Executed" | string; | |
| }; | |
| /** On-chain data related to the referenda */ | |
| onchainData: { | |
| /** Compare this to tx.method.hash.toHex() */ | |
| proposalHash: string; | |
| timeline: { | |
| _id: string; | |
| referendumIndex: number; | |
| name: "Executed" | "Confirmed" | "ConfirmStarted" | "DecisionStarted"; | |
| indexer: { | |
| blockHeight: number; | |
| /** Use this when generating the comment */ | |
| blockHash: string; | |
| blockTime: number; | |
| }; | |
| }[]; | |
| }; |
src/subsquare.ts
Outdated
| export class SubsquareApi { | ||
| private readonly url: string; | ||
| constructor() { | ||
| this.url = "https://collectives.subsquare.io"; | ||
| } | ||
|
|
||
| async fetchReferenda(index: number): Promise<ReferendaObject> { | ||
| const request = await fetch(`${this.url}/api/fellowship/referenda/${index}.json`); | ||
| return await request.json(); | ||
| } |
There was a problem hiding this comment.
The SubsquareApi class and its method fetchReferenda are correctly implemented for fetching referenda data. However, consider handling potential errors from the fetch call to improve robustness. For instance, checking if the request was successful before attempting to parse the JSON could prevent runtime errors if the API responds with an error status code.
async fetchReferenda(index: number): Promise<ReferendaObject> {
const request = await fetch(`${this.url}/api/fellowship/referenda/${index}.json`);
+ if (!request.ok) {
+ throw new Error(`Failed to fetch referenda data: ${request.statusText}`);
+ }
return await request.json();
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| export class SubsquareApi { | |
| private readonly url: string; | |
| constructor() { | |
| this.url = "https://collectives.subsquare.io"; | |
| } | |
| async fetchReferenda(index: number): Promise<ReferendaObject> { | |
| const request = await fetch(`${this.url}/api/fellowship/referenda/${index}.json`); | |
| return await request.json(); | |
| } | |
| export class SubsquareApi { | |
| private readonly url: string; | |
| constructor() { | |
| this.url = "https://collectives.subsquare.io"; | |
| } | |
| async fetchReferenda(index: number): Promise<ReferendaObject> { | |
| const request = await fetch(`${this.url}/api/fellowship/referenda/${index}.json`); | |
| if (!request.ok) { | |
| throw new Error(`Failed to fetch referenda data: ${request.statusText}`); | |
| } | |
| return await request.json(); | |
| } |
| You can set the GitHub action to also run notifying on a PR when a referenda is available for voting or it has closed. | ||
|
|
||
| It will look for new referendas available since the last time the action was run, so it won't generate duplicated messages. | ||
|
|
||
| If it finds an open PR that matches a completed referenda it will initiate the process to close it. | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The review comment correctly identifies the need to ensure that the GH_REPO environment variable is used within the action's logic. However, the verification process reveals that GH_REPO is mentioned in the README.md but not actively used in the action's logic or any other part of the codebase outside of markdown documentation. This discrepancy suggests that while the documentation provides setup instructions involving GH_REPO, the implementation does not utilize this variable, which could lead to confusion or misconfiguration.
Analysis chain
The addition of the notification job in the README.md file clearly explains the new functionality related to referenda notifications. It's well-documented and provides a good example of how to set up the GitHub action for this purpose. However, ensure that the GH_REPO environment variable mentioned in the script is actually used within the action's logic, as it's not clear from the provided context.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if GH_REPO is used in the action's logic
rg 'GH_REPO' --type md
Length of output: 120
Script:
#!/bin/bash
# Search for references to GH_REPO in non-markdown files to confirm its usage in the action's logic
rg 'GH_REPO' --type-not md
Length of output: 26
src/cron.ts
Outdated
| @@ -1,9 +1,11 @@ | |||
| import { debug, error, info, warning } from "@actions/core"; | |||
| import { summary, SummaryTableRow } from "@actions/core/lib/summary"; | |||
| import { ApiPromise, WsProvider } from "@polkadot/api"; | |||
| import { writeFile } from "fs/promises"; | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed that writeFile from fs/promises is imported in src/cron.ts but is not utilized within the file. This suggests that the import may be unnecessary or its intended use was not implemented.
Analysis chain
The addition of writeFile from fs/promises is noted. Ensure that this import is utilized within the file for file operations related to the action's functionality.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify usage of writeFile in src/cron.ts
rg 'writeFile' src/cron.ts
Length of output: 67
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/cron.ts (7 hunks)
Additional comments: 2
src/cron.ts (2)
- 7-7: The import of
SubsquareApiis correctly added to enable fetching referenda data. This aligns with the PR objectives.- 49-50: The introduction of
OpenReferendaandCompletedReferendatypes is a good practice for type safety and clarity in handling referenda data.
src/cron.ts
Outdated
| } | ||
|
|
||
| logger.info(`Available referendas: ${query}`); | ||
| const hashes: { url: string; hash: string }[] = []; | ||
| const ongoing: OpenReferenda[] = []; | ||
| const completed: CompletedReferenda[] = []; | ||
|
|
||
| const subsquareApi = new SubsquareApi(); | ||
| for (const index of Array.from(Array(query).keys())) { | ||
| logger.info(`Fetching elements ${index + 1}/${query}`); | ||
|
|
||
| const refQuery = (await api.query.fellowshipReferenda.referendumInfoFor(index)).toJSON() as { ongoing?: OnGoing }; | ||
| const refQuery = (await api.query.fellowshipReferenda.referendumInfoFor(index)).toJSON() as { | ||
| ongoing?: OnGoing; | ||
| approved?: (number | null)[]; | ||
| }; | ||
|
|
||
| if (refQuery.ongoing) { | ||
| logger.info(`Found ongoing request: ${JSON.stringify(refQuery)}`); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [52-117]
The getAllRFCRemarks function has been significantly updated to handle both ongoing and completed referenda. A few points to consider:
- Error handling is crucial for external API calls. Ensure that failures in fetching referenda data from
SubsquareApiare gracefully handled and logged. - The use of
Array.from(Array(query).keys())for iteration is valid but consider if there's a more intuitive approach for readability. - The final return structure
{ completed, ongoing }aligns with the function's updated purpose.
Consider adding more robust error handling around the SubsquareApi calls to improve fault tolerance.
try {
const rfc = await subsquareApi.fetchReferenda(index);
} catch (error) {
logger.error(`Failed to fetch referenda data for index ${index}: ${error}`);
continue;
}
src/cron.ts
Outdated
| for (const [pr, remark] of prRemarks) { | ||
| // We compare the hash to see if there is a match | ||
| const tx = api.tx.system.remark(remark); | ||
| const match = remarks.find(({ hash }) => hash === tx.method.hash.toHex() || hash === tx.method.toHex()); | ||
|
|
||
| // We first start with ongoing referendas | ||
| const match = ongoing.find(({ hash }) => hash === tx.method.hash.toHex() || hash === tx.method.toHex()); | ||
| if (match) { | ||
| logger.info(`Found existing referenda for PR #${pr}`); | ||
| const msg = `Voting for this referenda is **ongoing**.\n\nVote for it [here](${match.url})`; | ||
| rows.push([`${owner}/${repo}#${pr}`, `<a href="${match.url}">${match.url}</a>`]); | ||
| } | ||
|
|
||
| // if we don't find a match, we search for a closed referenda | ||
| const closedMatch = completed.find(({ hash }) => hash === tx.method.hash.toHex() || hash === tx.method.toHex()); | ||
| if (closedMatch) { | ||
| logger.info(`Found completed referenda for PR #${pr}`); | ||
| const msg = `/rfc process ${closedMatch.executedHash}`; | ||
| rows.push([ | ||
| `${owner}/${repo}#${pr}`, | ||
| `<a href="https://collectives.polkassembly.io/referenda/${closedMatch.index}>RFC ${closedMatch.index}</a>`, | ||
| ]); | ||
| await octokit.rest.issues.createComment({ owner, repo, issue_number: pr, body: msg }); | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [127-167]
The cron function has been updated to handle both ongoing and completed referenda. Points of interest:
- The logic to match PR remarks with referenda hashes is correctly implemented.
- The creation of comments on PRs for completed referenda is a valuable feature.
- Ensure that the URL in the comment for completed referenda is correctly formatted. There seems to be a missing quotation mark at the end of the URL.
There's a typo in the URL for completed referenda, causing a potential syntax error in the generated HTML.
- `<a href="https://collectives.polkassembly.io/referenda/${closedMatch.index}>RFC ${closedMatch.index}</a>`
+ `<a href="https://collectives.polkassembly.io/referenda/${closedMatch.index}">RFC ${closedMatch.index}</a>`
Bullrich
force-pushed
the
cron/close-completed-rfcs-prs
branch
from
2655eaa
to
982d416
Compare
This way, we don't execute it once it was already executed
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
dist/index.jsis excluded by:!dist/**
Files selected for processing (2)
- src/cron.test.ts (1 hunks)
- src/cron.ts (7 hunks)
Additional comments: 4
src/cron.test.ts (1)
- 5-6: The test modification correctly adapts to the updated
getAllRFCRemarksfunction's return type by destructuring the result and checking theongoingproperty. This change is logical and aligns with the function's updated purpose. In the future, consider adding more test cases to cover different scenarios, such as when there are ongoing or completed referenda.src/cron.ts (3)
- 7-7: The import of
SubsquareApiis correctly added to utilize it within thegetAllRFCRemarksfunction for fetching referenda data. This aligns with the PR objectives to enhance the GitHub action's interaction with referenda.- 49-50: The introduction of
OpenReferendaandCompletedReferendatypes is a good practice for type safety and clarity. These types accurately represent the structure of referenda data being handled, improving code maintainability and readability.- 156-183: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [136-180]
The updates to the
cronfunction to handle both ongoing and completed referenda are logically sound and align with the PR objectives. The logic for matching PR remarks with referenda hashes and creating appropriate comments is well-implemented. Ensure thorough testing to validate the matching logic and the creation of comments on PRs based on referenda status.However, ensure that the system is thoroughly tested, especially the logic that matches PR remarks with referenda hashes and creates comments on PRs based on the referenda status.
src/cron.ts
Outdated
| } | ||
| } else { | ||
| logger.debug(`Reference query is not ongoing: ${JSON.stringify(refQuery)}`); | ||
| if (refQuery.approved) { | ||
| logger.debug(`Fetching info from referenda ${index} from Subsquare`); | ||
| const rfc = await subsquareApi.fetchReferenda(index); | ||
| const confirmedBlock = rfc.onchainData.timeline.find(({ name }) => name === "Confirmed"); | ||
| if (confirmedBlock) { | ||
| const blockDate = await getBlockDate(confirmedBlock.indexer.blockHeight, api); | ||
| if (startDate > blockDate) { | ||
| logger.info(`Confirmation of referenda #${index} happened before the previous check. Ignoring.`); | ||
| continue; | ||
| } | ||
| completed.push({ | ||
| hash: rfc.onchainData.proposalHash, | ||
| executedHash: confirmedBlock.indexer.blockHash, | ||
| index, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| logger.info(`Found ${hashes.length} ongoing requests`); | ||
| logger.info(`Found ${ongoing.length} ongoing and ${completed.length} completed requests`); | ||
|
|
||
| return hashes; | ||
| return { completed, ongoing }; | ||
| } catch (err) { | ||
| logger.error("Error during exectuion"); | ||
| throw err; |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [52-126]
The getAllRFCRemarks function has been significantly updated to handle both ongoing and completed referenda. The logic is well-structured and aligns with the PR objectives. However, as previously noted, adding error handling around SubsquareApi calls is crucial for robustness. Additionally, consider simplifying the iteration logic for readability.
- For error handling, wrap the
subsquareApi.fetchReferenda(index)call in a try-catch block to gracefully handle potential failures. - The iteration using
Array.from(Array(query).keys())is valid but might be less intuitive for some developers. Consider using a more straightforward loop structure if readability is a concern.
+ try {
const rfc = await subsquareApi.fetchReferenda(index);
+ } catch (error) {
+ logger.error(`Failed to fetch referenda data for index ${index}: ${error}`);
+ continue;
+ }Consider simplifying the iteration logic for improved readability.
Now it also fetches completed RFCs from an older date
Doing this will stop this files of being tracked as diff but as binary changes, making the PRs cleaner. I also removed the slash from the test name because it appeared as `Check dist/ / Check dist/` in the action name which is a bit weird
# Conflicts: # dist/index.js
And added logging of queries which are not handled
This way the tests don't depend on it
Added the ability to check on open PRs if they match with a completed referenda.
If it matches, it will comment on it for the system to close it using the latest block from the timeline
Summary by CodeRabbit
SubsquareApiclass for fetching referenda data.