Open leighmcculloch opened 8 months ago
Thanks for drafting this, @leighmcculloch. I do agree that the APIs are suboptimal, but they were a huge improvement over building the XDR values from scratch at the time.
First, some rebuttals, and then alignment towards a solution:
const bigint = ScInt.fromScVal(scval);
Thanks for bringing this up, because it actually doesn't exist :sweat_smile: it's actually a documentation error (fixed in https://github.com/stellar/js-stellar-base/pull/723). So, on the brighter side, there are less ways.
Use the
XdrLargeInt
type. This type should probably not be exported as it seems like an implementation detail?
This is the lowest-level way to create the value and was specifically requested in the original implementation. Their difference is noted in the documentation:
new ScInt(bigi); // if you don't care about types, and new XdrLargeInt('i128', bigi); // if you do
I'm not sure I follow the arguments about XDR types: namespacing makes this clear. Nothing outside of the xdr
module is, well, raw XDR.
As a bonus, ScVal already contains functions for more advanced usage creating the values from parts:
The whole point of these abstractions was to NOT do this, because it's super duper error-prone and gross. 99.9% of developers will do it wrong (we literally did it wrong ourselves several times in the VM). And if you really want to, this is what XdrLargeInt
is for.
So, in summary, there really is only a single way to go from ScVal
to bigint
, which is scValToBigInt
. If you already have an ScInt
, it's different, of course. The inverse direction (bigint|number|string
to ScVal
) has more than one way, but imo this isn't really problematic: nativeToScVal
(generic) and ScInt
(specific) are on different abstraction layers.
Now, for the proposed APIs:
Functions on
ScVal
. Close proximity toScVal
so that the functionality is discoverable, and less new concepts (XdrLargeInt
,ScInt
) are introduced).
I think this is a great idea: it was mentioned in the original PR, but you can see in that thread that it's not exactly trivial to extend generated XDR, unfortunately.
Historically we've tried to pull people away from the XDR, and hide the XDR where possible behind nicer APIs. With Soroban folks need to embrace the XDR more than they did with classic operations. Also, in this case the APIs we've pulled people from to are still relatively confusing, and we can introduce less concepts by pushing people back to the XDR type and adding functions to simplify interaction on the XDR type.
I strongly disagree with embracing the XDR more if we can avoid it. The Soroban XDR structures are gross and error-prone and this is exacerbated by js-xdr
being completely unintuitive to interact with (aka _attributes
and all of that nonsense). I think many devs would agree that scValToNative
and nativeToScVal
are so much nicer than dealing with the generated code.
So I agree that attaching it to the structures would be the neatest approach, and I want to look into this more, but it's not a trivial fix unfortunately :/
Is your feature request related to a problem? Please describe.
The APIs for working with XDR ScVal number types are reasonably confusing and non-obvious. There are also multiple ways to do the same thing.
For example there are three ways to get at / work with number values:
Use the
scValToBigInt
function:Use the
ScInt
type:Another problem with
ScInt
is that it looks like an XDR type, but it isn't.Use the
XdrLargeInt
type. This type should probably not be exported as it seems like an implementation detail?Another problem with
XdrLargeInt
is that there's another typeLargeInt
which serves another purpose.Describe the solution you'd like
Functions on
ScVal
. Close proximity toScVal
so that the functionality is discoverable, and less new concepts (XdrLargeInt
,ScInt
) are introduced).As a bonus, ScVal already contains functions for more advanced usage creating the values from parts:
Or the simpler variants can be added:
Describe alternatives you've considered
Another option would be to focus on using just one of these methods, such as combining
ScInt
andXdrLargeInt
so all conversions (simple and advanced) are in one type, and removingscValToBigInt
since it's superfluous.Additional context
Thinking about this after seeing the following question in Discord:
Historically we've tried to pull people away from the XDR, and hide the XDR where possible behind nicer APIs. With Soroban folks need to embrace the XDR more than they did with classic operations. Also, in this case the APIs we've pulled people from to are still relatively confusing, and we can introduce less concepts by pushing people back to the XDR type and adding functions to simplify interaction on the XDR type.
(I am more interested in solving the problem than with the solution I suggest above, and definitely defer to others more qualified for better solutions.)