Initial discussion-point commit of proposed InterledgerAddress per IL-RFC 15 #18
Conversation
…C 15 * Updates InterledgerAdderss definition to conform to RFC 15 * Transitioned to Interface so multiple implementations of InterledgerAddress can exist. * Made InterledgerAddress immutable * Made InterledgerAddress implementation disallows null per https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained * Added unit tests
|
@sappenin this is great, thanks! One question, shouldn't it replace the class at: https://github.com/interledger/java-ilp-core/blob/development/src/main/java/org/interledger/ilp/InterledgerAddress.java ? |
|
@adrianhopebailie Yes, it should. I'll updated the PR via d037add |
| */ | ||
| private static final class InterledgerAddressImpl implements InterledgerAddress { | ||
|
|
||
| private static final String REGEX = "(?=^.{1,1023}$)^(g|private|example|peer|self|test[1-3])[.]([a-zA-Z0-9_~-]+[.])*([a-zA-Z0-9_~-]+)?$"; |
There was a problem hiding this comment.
Does anyone know why it was decided to only allow a certain number of schemes in an InterledgerAddress?
On the one hand, RFC 15 talks about InterledgerAddresses being used for routing, and not having any central authority (sort of like, anything can work in here, as long as it has these alphanumeric characters). But then it adds this restriction around scheme. Anyone know why?
Currently, this implementation doesn't allow a scheme that isn't in the Regex, but it's unclear to me what value the schemes add.
Perhaps this is a question for the mailing list?
There was a problem hiding this comment.
Perhaps this is a question for the mailing list?
Good idea
|
@adrianhopebailie In the previous version of InterledgerAddress, there is code relating to IA5 and translation from a byte[] into an InterledgerAddress. Does this need to be kept around? It's not clear to me why an InterledgerAddress would be created from a binary packet of data -- possibly from the binary data packet proposed in 168? |
| @@ -0,0 +1,70 @@ | |||
| package org.interledger.core; | |||
There was a problem hiding this comment.
This PR proposes that ILP-Java base classes (like InterledgerAddress.java, for example) be located in the org.interledger.core package.
The rationale is that something like org.interledger.ilp is somewhat redundant. For example, will the Interledger "community" ever publish something that's not ilp? It seems like the answer is no.
The only counter-point to this line of thinking is if the community (under the auspices of org.interledger) decided to publish an implementation of something like a ledger that doesn't support ILP. One might argue that in that case, the org.interledger.ledger package might need to be distinct from org.interledger.ilp.ledger, but I find this type of possibility unlikely, for the primary reason that if somebody wants to be publishing non-ILP components, then probably the org.interledger package is not the appropriate place to do that.
Thoughts?
There was a problem hiding this comment.
My rationale for the package naming was that I see org.interledger as the project prefix. There are many aspects to "Interledger" including the ILP protocol but also other protocols like SPSP.
So stuff that is core ILP is in the org.interledger.ilp namespace.
There was a problem hiding this comment.
Oh interesting, thanks for clarifying - that makes sense.
I'm open to reverting the package name, but my only remaining concern is that "ILP" and "Interledger Protocol" are semantically equivalent, but we're trying make them mean two different things.
More specifically, the term "ILP" is actually an acronym that expands to "Interledger Protocol" (I think). So, on the one hand we're saying a protocol like "SPSP" (RFC-9) is not "ILP" (RFC-3), yet we're also saying that SPSP is an "Interledger Protocol" or "ILP."
Seems like conceiving of the thing defined in RFC-3 as something like "Interledger Core" or "Interledger Core Protocol" would then allow us to say that SPSP, ILQP, etc are all ILP/Interledger.
Thoughts?
(I'm hesitant to be bike-shedding here, so feel free to respond back with 'hey, revert the package name and log a ticket to discuss later, or never'. I'm just trying form a mental model of how to organize our code.)
There was a problem hiding this comment.
we're also saying that SPSP is an "Interledger Protocol" or "ILP."
I'd disagree with this. SPSP is a setup protocol and a setup protocol is a prerequisite for making a payment using ILP. i.e. Before you can make an ILP payment you need to know what the ILP Address and amount are that you put in the packet and also the condition to attach to the first transfer.
It's somewhat analogous to DNS vs IP in my mind.
| * | ||
| * @see "https://github.com/interledger/rfcs/tree/master/0015-ilp-addresses" | ||
| */ | ||
| public interface InterledgerAddress { |
There was a problem hiding this comment.
Since we have various teams of people implementing ILP in Java using various types of libraries, this PR proposes to make all of our "core" classes interfaces, so that if somebody doesn't like the community-provided implementation, they would be free to create their own version while being able to utilize it in existing code bases. Hopefully there's no disagreement here.
* Remove old InterledgerAddress class (and unused unit tests) * Update Javadoc to abide by checkstyle * Add package-info.java
mDuo13
changed the title
* Move core classes back to org.interledger.ilp package per code review comments. * Improve Javadoc comments. * Add more unit tests.
|
I think the only remaining blocker for committing this PR was the discussion around whether or not to maintain the core package naming of Thus, I've moved the |
|
Successful Build here. |
|
Nice job! |
…-RFC 15 (#18) * Initial discussion-point commit of proposed InterledgerAddress per RFC 15 * Updates InterledgerAdderss definition to conform to RFC 15 * Transitioned to Interface so multiple implementations of InterledgerAddress can exist. * Made InterledgerAddress immutable * Made InterledgerAddress implementation disallows null per https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained * Added unit tests * Remove old InterledgerAddress class * Remove old InterledgerAddress class (and unused unit tests) * Update Javadoc to abide by checkstyle * Add package-info.java * Misc changes per code review comments * Move core classes back to org.interledger.ilp package per code review comments. * Improve Javadoc comments. * Add more unit tests.
Starting-point PR to discuss a new InterledgerAddress...