add jss fields used by clio nft_info

[ledhed2222]

High Level Overview of Change

The new nft_info API that exists in clio only added some fields that don't exist in our JSS enum. Adding them to prevent future confusion/collision.

Context of Change

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Tests (You added tests for code that already exists, or your new feature included in this PR)
• Documentation Updates
• Release

[seelabs]

It looks like this is adding constants to ripple that aren't used in rippled. Since these are used in clio I'd suggest adding them to clio instead. If they end up being moved to rippled it can be removed from clio and added to rippled at that point.

[ledhed2222]

@cjcobb23

[scottschurr]

I think this pull request is a great way to elevate the discussion about how rippled and clio interoperate and remain synchronized over the years. So thanks for that, @ledhed2222.

I'm gonna get long winded. Sorry. It's my way.

How Does clio Relate to rippled?

I'm going to state my understanding about how clio and ripple relate to one another. That understanding my be wrong, so I welcome being corrected.

1. In essence, clio is designed to be a query handler for the sorts of questions one might ask rippled...
   1. clio's data storage is more efficient than rippled's
   2. clio is not trying to validate ledgers, so it can dedicate its resources to responding to API queries.
2. Because clio responds to the sort of questions one might ask rippled...
   1. Users want rippled and clio to have the same query API and
   2. They want query responses from clio and rippled to result in identical JSON.

If we succeed at that, then users of rippled and clio don't need to distinguish between whether they call clio or rippled. This is a big win for client usability. But if clio and rippled start to diverge, then it gets harder for users to ignore the choice of whether they are calling rippled or clio.

Responses From clio and rippled Are Now Intentionally Diverging

This pull request is a consequence of an intentional divergence from what I've just described. Rather than providing the same API for clio and rippled, clio now provides a superset of capabilities. The divergence is starting small, but it's there.

The divergence is happening because there are questions about the state of the ledger that would be expensive for rippled to answer. clio can answer some of these questions more efficiently than rippled could, so it makes sense to add those capabilities to clio.

So at this new point, a user who needs the information only clio can provide must connect to clio. Other users can connect to either rippled or clio.

What we see in this pull request is the camel's nose of the upcoming divergences. This one is intentional. There will probably be other divergences in the future that are both intentional and unintentional. Unintentional divergences will occur because both clio and rippled are live, evolving, and largely independent code bases.

How Should We Handle Divergences Between clio and rippled?

The question at hand is how do we want to handle these divergences so that...

1. We get the divergences that we want,
2. The divergences we produce are harmonious (the divergences produce JSON that plays well together),
3. Our processes resist us producing unintentional divergences, and
4. The costs associated with these assurances are acceptable to the developers and to the company.

I think this pull request is a reasonable first stab in starting to handle these divergences. JSON has three compatibility legs it stands on:

1. In its keyword : value structure, within a given schema a keyword should mean the same thing and not be subject to unintentional spelling/capitalization/pluralization variations.
2. The types of values returned for a given keyword should be consistent.
3. The larger JSON object/array structure should be consistent. Inner objects should appear at known and expected places within the object hierarchy.

So What Does It Mean for This Pull Request?

This pull request is attempting to address the keyword consistency problem -- number 1 above. What does the pull request actually accomplish?

• It helps encourage clio and rippled to spell JSON keywords the same way.

To be fair, that's all jss.h can do, even inside of rippled. And jss.h has been a boon within rippled. Within rippled we have largely replaced the quoted c-strings originally used for JSON reading and writing with manifest constants defined in jss.h. This is a great thing. The jss:: constant must be present and spelled correctly, including capitalization, or you get a compile failure. jss.h also provides us a self-maintaining library of keyword values used in our returned or consumed JSON. If I'm cooking up new JSON, I can look in jss.h and it tells me which keywords have already been used. I don't have to scan the whole code base and hope I didn't miss anything.

There are downsides to the change.

1. jss.h is part of rippled. A clio developer must now modify rippled before they can make the new keyword available to the clio codebase. And now that new keyword is unavailable to the clio codebase until the change to rippled has been released. This reduces velocity for clio code development.
2. The change only helps verify keyword spelling. It doesn't help with other issues that might occur:
   • The keyword is spelled the same but the meaning is different.
   • The keyword is spelled the same, but the value associated with the keyword is inconsistent (quoted decimal numbers anyone?).
   • The keyword is the same, but the structure of the JSON is different: the keyword or object shows up in the wrong place.

Even with the given limitations, I think this is probably a good change.

1. The suggested change came from a clio developer. So I assume clio developers are willing to put up with the hit to their own development velocity.
2. Nobody is talking about either...
   1. A shared JSON checker between rippled and clio to verify that they build sufficiently similar JSON, or
   2. A shared JSON formatter, so the JSON they produce is similar by construction.

If someone were talking about a shared JSON checker or formatter then this change might be less useful. At the least, this change would want to be made in the context of that larger change.

But, as it stands, the downsides to this change seem small, at least for rippled development. The benefits are also small, but they are real.

I'm hoping this long-winded exposition is the start of a discussion. So I'm not approving the pull request yet, even though I think the change is good. I'd like to hear other opinions and alternatives before I make up my mind. Thanks.

[ledhed2222]

@scottschurr love love love your comment!

I'm going to state my understanding about how clio and ripple relate to one another. That understanding my be wrong, so I welcome being corrected.

In essence, clio is designed to be a query handler for the sorts of questions one might ask rippled...
    clio's data storage is more efficient than rippled's
    clio is not trying to validate ledgers, so it can dedicate its resources to responding to API queries.
Because clio responds to the sort of questions one might ask rippled...
    Users want rippled and clio to have the same query API and
    They want query responses from clio and rippled to result in identical JSON.

So, the more I deal with clio, and this is from someone that is not on the clio team per se, I have a different perspective. I think that clio is now a superset of rippled APIs. And ideally what we have moving forward is that clio is the way you talk to your rippled client, only. So for that reason, I see more divergences coming, unless we start thinking about rippled's API as soon-to-be frozen/deprecated/hopefully deprecated etc.

If we succeed at that, then users of rippled and clio don't need to distinguish between whether they call clio or rippled. This is a big win for client usability. But if clio and rippled start to diverge, then it gets harder for users to ignore the choice of whether they are calling rippled or clio.

You're totally right. Users should not have to know about clio. They should just connect and see an API. I think we're at the start of a transition period, but in the end users will just connect to clio because it provides additional APIs that (in the case of NFTs) are necessary for any actual product. IE - I think if we do this right then user's won't know about clio, they'll just know that there is an API, but that API is clio, and not rippled.

The divergence is happening because there are questions about the state of the ledger that would be expensive for rippled to answer. clio can answer some of these questions more efficiently than rippled could, so it makes sense to add those capabilities to clio.

So at this new point, a user who needs the information only clio can provide must connect to clio. Other users can connect to either rippled or clio.

What we see in this pull request is the camel's nose of the upcoming divergences. This one is intentional. There will probably be other divergences in the future that are both intentional and unintentional. Unintentional divergences will occur because both clio and rippled are live, evolving, and largely independent code bases

Ah yeah - you get it.

[ledhed2222]

so - @scottschurr and @cjcobb23 and @seelabs , another way i could look at this (macro level) is to say that this file, in particular, needs to be its own repo. I know that sucks but that might be the truly safe and correct thing to do.

[seelabs]

Is the argument that clio has RPC commands that has fields that rippled doesn't have, and those fields may someday be put into rippled, so let's put them there now in case that happens to make sure they use the same name as clio? If that's the argument, then I'm still opposed. I also don't think this needs to be it's own repo. Things that are common between the two projects belong in rippled. Things that are unique to clio belong in clio.

[scottschurr]

Is the argument that clio has RPC commands that has fields that rippled doesn't have, and those fields may someday be put into rippled, so let's put them there now in case that happens to make sure they use the same name as clio?

Yeah, I guess that's the point. rippled and clio must use a common vocabulary of keywords in their API. clio doesn't necessarily use the full vocabulary (defined in jss.h) that rippled uses, but it still has that full vocabulary available. To me It seems like rippled should also have the full vocabulary that clio uses available.

Let me try it this way. From a naive perspective, rippled and clio appear to be (largely) independent programs. But they are not. clio's job is to return a superset of data that rippled returns through its API. The overlap of the APIs of rippled and clio must, at each step, stay as similar to each other as possible for the very long term. If the overlapping APIs stray from each other then users trying to get rippled history get fouled up.

Both rippled and clio are under active development, and will be into the future. As such, API keywords that rippled uses should be the same keywords that clio uses. If clio introduces new keywords in its superset API, then rippled programmers should be aware of those keywords. If rippled programmers are aware of those keywords, then they can either use them (intentionally) or avoid them as rippled is modified into the future. It reduces the likelihood of rippled programmers introducing minor variations on names used by clio.

So this pull request is intended as a first step in the goal of keeping rippled and clio's APIs as similar as possible. And I think this pull request helps a little bit in that quest.

Please note that, at the moment, clio seems to be the only long term solution that rippled has for full history. So the usability of clio is vital to the long term viability of rippled. To some extent we cannot view them as independent programs. We should instead view them as complementary programs.

[ximinez]

If rippled programmers are aware of those keywords, then they can either use them (intentionally) or avoid them as rippled is modified into the future. It reduces the likelihood of rippled programmers introducing minor variations on names used by clio.

If the goal is to ensure rippled developers know about this field, my suggestion is to add these JSS variables commented out (in their proper place), with a comment next to each one explaining the relevance to clio, and a note that if one uncomments the item, they'll need to coordinate with clio developers. Maybe clio could use some kind of precompiler flag to define the variables conditionally, so the coordination becomes easiler.

[scottschurr]

@ximinez, that's an interesting idea. Maybe add a JSS_CLIO macro that only defines the constant if we're compiling for clio? We're already using macros in the file...

[ledhed2222]

Thinking about this more, this PR is a bad idea because it will tightly couple clio and rippled (bidirectionally). If we actually wanted to do this, we should put these sort of shared values, likely including error codes, into a separate repo that is linked to each project. Clearly that's not going to happen any time soon.

Thoughts?

[ximinez]

Thinking about this more, this PR is a bad idea because it will tightly couple clio and rippled (bidirectionally). If we actually wanted to do this, we should put these sort of shared values, likely including error codes, into a separate repo that is linked to each project. Clearly that's not going to happen any time soon.

Thoughts?

If the value was shared and used bidirectionally, then another repo might make sense, but what I'm seeing is clio using a value, and just wanting to make sure that rippled doesn't step on its toes. Not the same.

[seelabs]

I'd love it if rippled only had code that was necessary for running the rippled server. So, as little RPC as possible, no special "reporting" modes, and no common code or constants that aren't used in rippled (including compiled out macros). This moves us away from that. I don't see clio and rippled as joined. Clio is built on top of rippled.

Having said that, while I oppose this, I only weakly oppose it. This isn't a hill I'm going to die on :-) It's not that big of a deal if we added these constants. I think as long as no one "strongly" opposes this (and I only "weakly" oppose) and other people think this is the way to go, I'm Ok merging this.

Also: When the heck is github going to add threaded top-level comments.

Edit: I understand the argument about a "common vocabulary", and it's a valid point. I guess I'm less worried about that because I see clio as the home for new RPC commands. The most likely new vocabulary that will appear in rippled will involve new ledger objects and transactions - and those new JSS constants will appear in rippled before clio.

[scottschurr]

I'd love it if rippled only had code that was necessary for running the rippled server. So, as little RPC as possible, no special "reporting" modes, and no common code or constants that aren't used in rippled (including compiled out macros). This moves us away from that. I don't see clio and rippled as joined. Clio is built on top of rippled.

Yup. We're in heated agreement. And I agree, this change moves us (slightly) away from the desired direction.

The problem is that we don't really have a plan for getting to that desirable place. We need to do the best we can with what we have right now. What I want to avoid is getting to the (nearly) disastrous place that gRPC took us a few years ago. I think @ledhed2222 has a view of the upcoming API synchronization issues between clio and rippled. And I want to help him get whatever tools he needs to keep us from digging ourselves into that same (gRPC-like) hole yet again.

I think this is just the first step on the journey. This change may be backed out once we understand the landscape better. I'd be fine with that.

Edit: I understand the argument about a "common vocabulary", and it's a valid point. I guess I'm less worried about that because I see clio as the home for new RPC commands. The most likely new vocabulary that will appear in rippled will involve new ledger objects and transactions - and those new JSS constants will appear in rippled before clio.

I almost agree with that. clio really is claiming new keywords in the API namespace. And rippled will be claiming new keywords too. If clio has already claimed a keyword, and rippled developers don't have visibility of that, rippled developers can unknowingly claim a keyword that clio has already claimed. Or worse, can unknowingly claim a keyword that is almost the same as a keyword that clio has already claimed.

Since the API keyword namespace is shared, I think both clio and rippled developers need visibility of the full (both rippled and clio) namespace. Maybe later we can find a way to partition the API namespace? But right now we are where we are.

But I don't think I have any better viewpoint than anyone else. I'd be interested to hear what @ledhed2222 and @ximinez think about this pull request now.

[ximinez]

But I don't think I have any better viewpoint than anyone else. I'd be interested to hear what @ledhed2222 and @ximinez think about this pull request now.

I'm neutral toward weakly in favor of this PR, so long as it's annotated that these fields are needed by clio (which they are). I don't think there is a really optimal way to do what we want to do, and this seems like the lesser evil.

[intelliot]

(I only removed the old Needs Review label because we don't use that label anymore. I believe this still needs review, unless @ledhed2222 thinks otherwise.)

[shawnxie999]

Looks good

[seelabs]

👍 LGTM

[intelliot]

@ledhed2222 this now has sufficient approvals. If you agree that this is ready to merge now, you can apply the Passed label, or just comment here saying that it's ready.

[scottschurr]

👍 LGTM

[intelliot]

@ledhed2222

1. Why was this branch force-pushed? I see there are no changes between the different commits, so why bother?
2. Please review the following commit message, and either confirm that it looks good to you, or propose an alternative:

Add jss fields used by Clio `nft_info`: (#4320)

Add Clio-specific JSS constants to ensure a common vocabulary of
keywords in Clio and this project. By providing visibility of the full
API keyword namespace, it reduces the likelihood of developers
introducing minor variations on names used by Clio, or unknowingly
claiming a keyword that Clio has already claimed. This change moves this
project slightly away from having only the code necessary for running
the core server, but it is a step toward the goal of keeping this
server's and Clio's APIs similar. The added JSS constants are annotated
to indicate their relevance to Clio.

Clio can be found here: https://github.com/XRPLF/clio

[ledhed2222]

@intelliot i force pushed just to add my signature to the commit.

yes - love the commit message!