Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full custom definition support #2692

Open
mvadari opened this issue May 6, 2024 · 5 comments · May be fixed by #2683
Open

Full custom definition support #2692

mvadari opened this issue May 6, 2024 · 5 comments · May be fixed by #2683

Comments

@mvadari
Copy link
Collaborator

mvadari commented May 6, 2024

It'd be much easier to use the same library with different sidechains that support different features, since it affects serialization and signing capabilities.

Some work already done on this front:

The only remaining step is to add support for the server_definitions RPC method - to call it on connection and store/use it if it is different than the stored file.

@mvadari
Copy link
Collaborator Author

mvadari commented May 6, 2024

One other thought: we could potentially shrink the binary codec significantly with the complete removal of the definitions.json file entirely, relying entirely on the server_definitions RPC. This may not be worth it due to the corresponding reduction in ease-of-use.

@ckeshava
Copy link
Collaborator

ckeshava commented May 6, 2024

@mvadari Yes, I agree with you. Regarding removing the definitions.json file entirely: we will need to perform an RPC call once to fetch the server_definitions output and then use it to perform all forms of serialization/de-serialization right?

I suspect the initial latency might be high (due to fetching the server_definitions), but subsequent operations should be fast.

Are there any other concerns? I believe there are many rippled servers that provide a public-API

@dangell7
Copy link
Contributor

The issue with this is that in this repo we have models and validation. So we could use the binary codec and not use the transaction model however i think this results in errors when signing so you end up doing a lot of @ts-ignore.

Also requests sorta break this from a sidechain issue as we also have requests and objects that need to be parsed specifically for that chain.

If you really wanted to make this an agnostic repo, exporting the transactions, with their required/optional values could help you build a custom model but I dont think typescript can do this runtime, but maybe an automated github task to create different models for different sidechains. I actually tree shook the models here if you're interested in what that would look like. We can actually use these models in the xrpl-accountlib library for those who like to keep everything typed.

@mvadari
Copy link
Collaborator Author

mvadari commented May 14, 2024

The issue with this is that in this repo we have models and validation. So we could use the binary codec and not use the transaction model however i think this results in errors when signing so you end up doing a lot of @ts-ignore.

If you're strictly only using the binary codec you should be fine, because the binary codec doesn't use those models. If that's not true, then that's a (minor) bug.

Also requests sorta break this from a sidechain issue as we also have requests and objects that need to be parsed specifically for that chain.

We can't solve all the problems, lol. But maybe that can be mostly resolved via @ts-ignore, or perhaps there can be some sort of support for custom requests.

If you really wanted to make this an agnostic repo, exporting the transactions, with their required/optional values could help you build a custom model but I dont think typescript can do this runtime, but maybe an automated github task to create different models for different sidechains. I actually tree shook the models here if you're interested in what that would look like. We can actually use these models in the xrpl-accountlib library for those who like to keep everything typed.

There's been some previous discussion about tree-shaking (see #2307 for example), but I think it hasn't been high priority so far.

@tequdev
Copy link
Contributor

tequdev commented Aug 16, 2024

Definitions support for client.submit() and client.submitAndWait() is also required. (internally, getSignedTx() and isSigned())

@mvadari mvadari linked a pull request Aug 28, 2024 that will close this issue
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants