Skip to content
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

meta: diff checker for CAPs #893

Closed
leighmcculloch opened this issue Mar 16, 2021 · 6 comments · Fixed by #950
Closed

meta: diff checker for CAPs #893

leighmcculloch opened this issue Mar 16, 2021 · 6 comments · Fixed by #950
Assignees

Comments

@leighmcculloch
Copy link
Member

leighmcculloch commented Mar 16, 2021

I think we could benefit from a small scoped investment in a diff checker on CAPs.

On two or three occasions diffs have been committed that are corrupt. This is usually discovered by someone consuming the diff, and not by the person publishing the diff. Diffs are fiddly and it's easy for copy paste errors to occur, or stray characters, or missing new lines at the end to result in a corrupt diff.

We could build a small utility that runs in a GitHub Action that:

  1. Grabs all CAPs in a PR that contain a diff block that begins with ```diff and ends with ```
  2. Extract the diff within the block, minus its last new line (see CAP-0021: Fix diff #892 for why the last line should be ignored).
  3. Checks out the stellar-core at the version of stellar-core the CAP provides (requires a new meta entry in CAPs to define what protocol they are proposed against).
  4. Attempts to run git apply, and posts any errors to the PR, notifying the diff author it is corrupt.

cc @MonsieurNicolas @stanford-scs @jonjove @sisuresh

@github-actions
Copy link

This issue is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.

@github-actions github-actions bot added the stale label Apr 16, 2021
@leighmcculloch
Copy link
Member Author

@MonsieurNicolas Are you open to receiving a PR to this repository that adds something minimal that does this ☝🏻?

@github-actions github-actions bot removed the stale label Apr 17, 2021
@MonsieurNicolas
Copy link
Contributor

yeah sounds good if it's not too much trouble!

re: what do we diff against, I'd say to just use git tag names, like v16.0.0 that is release 16.0.0 as humans can at a glance tell what the reference version is.

Meta data can just be inside the quoted block to avoid interaction with markdown, maybe something like (the patch is copy/pasted as is between the begin/end lines):

<<<< BEGIN_PATCH ref=v16.0.0
diff --git a/src/xdr/Stellar-overlay.x b/src/xdr/Stellar-overlay.x
index a4ab6b077..8fc14a1dd 100644
--- a/src/xdr/Stellar-overlay.x
+++ b/src/xdr/Stellar-overlay.x
@@ -93,7 +93,9 @@ enum MessageType
     HELLO = 13,

     SURVEY_REQUEST = 14,
-    SURVEY_RESPONSE = 15
+    SURVEY_RESPONSE = 15,
+
+    RESERVED_INTERNAL_MSG = 2147483647
 };

 struct DontHave
diff --git a/src/xdr/Stellar-transaction.x b/src/xdr/Stellar-transaction.x
index 75f39eb4e..30b1505e0 100644
--- a/src/xdr/Stellar-transaction.x
+++ b/src/xdr/Stellar-transaction.x
@@ -49,7 +49,8 @@ enum OperationType
     REVOKE_SPONSORSHIP = 18,
     CLAWBACK = 19,
     CLAWBACK_CLAIMABLE_BALANCE = 20,
-    SET_TRUST_LINE_FLAGS = 21
+    SET_TRUST_LINE_FLAGS = 21,
+    RESERVED_INTERNAL_OP = 2147483647
 };

 /* CreateAccount
>>>> END_PATCH

@leighmcculloch
Copy link
Member Author

I think we should avoid modifying the contents of the code block, because that prevents us from using the diff GitHub markdown modifier to color code the diff. I think we should find somewhere else to put the version it is based on.

We could put it immediately prior to the block in some consistent format to keep it close to the diff.

The reason I suggested adding it to the metadata is because CAPs are proposed diffs to a specific version of the protocol, so this could be useful information to have for a proposal as a whole.

@github-actions
Copy link

This issue is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.

@leighmcculloch
Copy link
Member Author

leighmcculloch commented May 25, 2021

I created mddiffcheck, a tool that will do this: https://github.com/stellar/mddiffcheck. There's a draft PR adding a GitHub Action for it in #950.

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 a pull request may close this issue.

2 participants