XRPLF / xrpl4j

A 100% Java implementation to interact with the XRP Ledger.
ISC License
86 stars 48 forks source link

Re: Do not fail in missing attributes. #130

Closed francisrosario closed 3 years ago

francisrosario commented 3 years ago

Hello! :), Is there any way to configure https://github.com/XRPLF/xrpl4j/blob/main/xrpl4j-model/src/main/java/org/xrpl/xrpl4j/model/jackson/ObjectMapperFactory.java not to fail on 'missing attributes' ... I did tried different combination of JsonMapper .configure but it still fails.

This is for the xumm4j SDK that I made: https://github.com/intelliot/xrpl-ideas/discussions/19 XUMM API doesn't need .account and .sequence

Pretty much really needed some help 'cause It will be redundant If I publish a modified version of xrpl4j model where account and sequence are marked as Optional<?> in Transaction.java

Example

        Payment payment = Payment.builder()
                //.account()
                //.sequence()
                .fee(XrpCurrencyAmount.ofDrops(12))
                .destination(Address.of("ra5nK24KXen9AHvsdFTKHSANinZseWnPcX"))
                .amount(XrpCurrencyAmount.ofXrp(BigDecimal.valueOf(8787)))
                .build();

This would fail since .account is missing and .sequence image

nkramer44 commented 3 years ago

Hey @francisrosario, this exception isn't actually coming from the Jackson ObjectMapper. The Payment class (and all Transactions) has certain required fields, such as account and sequence. When the ObjectMapper tries to deserialize JSON to a Payment, it will construct a Payment with all available fields. Once that Payment gets constructed, the Payment constructor will throw this IllegalStateException because the account and sequence fields are missing.

I'm hesitant to make account and sequence both Optional fields for a couple of reasons: 1) For XRPL transactions, these fields are both required in all situations. xrpl4j is an XRPL library, so the model objects we provide should match the contracts of the XRP Ledger protocol. 2) Optional fields are not very nice to deal with in Java. Adding more Optional parameters to this API would create confusion for developers, especially as they look at xrpl.org docs and try to map them to xrpl4j objects and because in all cases they will be present (as long as you're dealing with rippled).

I think it would be a mistake to make modifications to any objects in xrpl4j just for the purposes of a XUMM SDK. While the XUMM transaction templates share a lot of commonalities with XRPL transactions, they are a fundamentally different API.

Instead, I would write your own model objects to map XUMM API JSON to Java objects. We have done this in the past and it actually provides for better separation of code.

sappenin commented 3 years ago

Another option here is to just hardcode constants for those two fields in your own code (no changes needed in the xrpl4j code base) - if XummApi doesn't need thise two fields, then they should be ignored by Xumm right (even if specified)?

francisrosario commented 3 years ago

Another option here is to just hardcode constants for those two fields in your own code (no changes needed in the xrpl4j code base) - if XummApi doesn't need thise two fields, then they should be ignored by Xumm right (even if specified)?

Works good now, Thanks everyone...

image

sappenin commented 3 years ago

@francisrosario Just double-checking here - the screenshot you posted doesn't include any hard-coded constants from your side (e.g., for .account(...) or .sequence(...)), so its unclear how that code would work. I also wanted to make sure you figured out a way to make the Payment transaction work as well.

Perhaps you hard-coded values in your forked code, but I was suggesting that you hard-code values in your application code so you don't have to use a fork of xrpl-4j.

Thoughts?

francisrosario commented 3 years ago

@francisrosario Just double-checking here - the screenshot you posted doesn't include any hard-coded constants from your side (e.g., for .account(...) or .sequence(...)), so its unclear how that code would work. I also wanted to make sure you figured out a way to make the Payment transaction work as well.

Perhaps you hard-coded values in your forked code, but I was suggesting that you hard-code values in your application code so you don't have to use a fork of xrpl-4j.

Thoughts?

Yep! I can hard-code those values for all Transactions like payment, AccountSet similar to this kind of implementation I've provided below. I'll add this to my to-do list. Right now, I'm busy doing some of my other XRPL related projects. If you have any other best solution other than the code I provided below, please suggest It would be a big help ;).

// With this I don't need a fork of xrpl4j.
    public Payment payment(String destination, BigDecimal amount){
        Payment payment = Payment.builder()
                .account(Address.of("SOME_RANDOM_CONSTANTS_OR_EVEN_BLANK"))
                .sequence(UnsignedInteger.valueOf("SOME_RANDOM_CONSTANTS_OR_EVEN_BLANK"))
                // Account and sequence will be omitted once sent to XUMM API.
                .destination(Address.of(destination))
                .fee(XrpCurrencyAmount.ofDrops(12))
                .amount(XrpCurrencyAmount.ofXrp(amount))
                .build();
        return payment;
    }

Thanks, @nkramer44 and @sappenin, for the quick support and advice ;)... I'll be posting more issues here hahaha.. 'cause I'll be active in XRPL development.