rchain / rchip-proposals

Where RChain improvement proposals can be submitted
Apache License 2.0
8 stars 5 forks source link

RCHIP-01: Add bytesToHex method #11

Open SteveHenley opened 4 years ago

SteveHenley commented 4 years ago

Specify expressions for string (Aurthur Greef PR merge - convert into RChip) Dappy needs Scala regular expressions to be exposed in Rholang - e.g. to check for domain names

https://www.tutorialspoint.com/scala/scala_regular_expressions.htm

SteveHenley commented 4 years ago

The original intent of this RChip 09 was to address the implementation of bytesToHex.
Does the scope of this RChip need to be narrowed so that it specifically addresses bytesToHex?

Add bytesToHex method #2918

tgrospic commented 4 years ago

@SteveHenley bytesToHex is only applicable to Rholang byte array so it should not be mixed with additional methods for regular expressions on strings.

dckc commented 4 years ago

I see this is cited by https://github.com/rchain/rchain/pull/2918 please reduce the scope and update the title for bytesToHex only, @SteveHenley .

Also, this seems to be missing much of the information required by the RCHIP draft from the 2018 annual meeting:

I don't suppose that was ever ratified, but I haven't seen anything that supersedes it.

Who is on the Editorial Committee? the Approval Committee?

p.s. for follow-up on process, see #14

SteveHenley commented 4 years ago

To clarify, if you are request that the title for RChip 09 be updates to byesToHex only, then here is Rao's reply: "bytestoHex is one of the string functions. So we need only one. Exposing Scala regular expressions is the current agreed to scope of 'Improve string functions'". https://canary.discordapp.com/channels/375365542359465989/393462637100400650/711016913186324529

There is currently no editorial committee nor approval committee. Due to the limited manpower the tech governance working group decided to go with lighter version of the RChip. Rao can better explain what this lighter version is. @dckc

dckc commented 4 years ago

https://github.com/rchain/rchain/pull/2918 doesn't cover Exposing Scala regular expressions. It's just bytesToHex. For sanity's sake, please either expand the scope of the PR or narrow the scope this issue. The latter sure seems easier.

As to "agreed scope", please cite the decision record. I don't believe anyone has mandate to make such decisions on behalf of the coop or even on behalf of me.

jimscarver commented 4 years ago

To avoid ReDoS attacks and pricing issues with regular expressions we can provide basic pattern matching by supporting unix file glob patterns. https://en.wikipedia.org/wiki/Glob_(programming)

Regular expression might best be implimented as rholang patterns rather than strings in a separate RChip

jimscarver commented 4 years ago

Given the lack of full regular expression support a frequent requirement is for tolowercase(String) so that rchain can support case insensitive strings and "Jim" tolowercase can be compared to be equal to "jim".

SteveHenley commented 4 years ago

As a result of the last tech governance session discussionsI have updated the title of RChip-09 to Improve string function: bytesToHex so that it is more narrow in focus.

SteveHenley commented 4 years ago

Update title to be more concise based comments above and tech governance discussion.

SteveHenley commented 4 years ago

I have created a google docs RChip-09-bytesToHex for us to collaborate. The doc uses two templates, at the top there is "Specify the Features" which is from the original RChip process. At "History" we have the second template which the SIP. This is our first attempt at designing a template we want to use. The template we standardize on will eventually get narrowed down.

dckc commented 4 years ago

@tgrospic I just noticed the Approved label here; that's not a judgement you made unilaterally, right? I suspect the decision was made by the tech governance group. Please cite the record of that decision. (Or make a record of it here, noting when it was made and who participated. Enumerating yes / no / abstain is traditional too)

tgrospic commented 4 years ago

@dckc I added label Approved after PR was merged. I agreed to approve the PR after decision on the tech governance group. We decided to be less demanding on the proposal description because working code is already created. I'm not aware that we have written record. @SteveHenley, @9rb can you help me here?

I suggested that we use emoticons (:+1: :-1:) in the description (?).

SteveHenley commented 4 years ago

@dckc Please refer to the Log: Tech Governance, date 2020-06-25, agenda item #4 regarding the approval of RCHIP-01: Add byesToHex Method and all subsequence RCHIPs and RCHIP issues.

dckc commented 4 years ago

OK. If everything is approved, I trust you'll get rid of the Approved label, since it doesn't mean anything.