-
Couldn't load subscription status.
- Fork 139
Add support for encoding Address to/from new StrKeys
#801
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
Conversation
Shaptic
commented
May 20, 2025
- LPs, CBs, and muxed accounts now can be converted:
- from strkey -> Address
- Address -> ScVal
- Address -> ScAddress
- ScVal -> Address
|
Size Change: +24.4 kB (+0.72%) Total Size: 3.44 MB
|
| opts.func.switch().value === | ||
| xdr.HostFunctionType.hostFunctionTypeInvokeContract().value | ||
| ) { | ||
| // Ensure that there are no claimable balance or liquidity pool IDs in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if this validation is worthwhile as convenience for clients or is it possibly too opinionated for this layer. Maybe the contract fn wants to receive cb/lp addresses as opaque data of which that intent is not known here at this layer and appears like it could inadvertently trip on such cases?
If this validation were removed where would the problem with cb/lp addresses occur further downstream? Would simulation catch it? Is it feasible to let the lower downstream layer be the source of truth rather than duplicating some of that rule here in js layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the contract fn wants to receive cb/lp addresses as opaque data
It can't/shouldn't.
Is it feasible to let the lower downstream layer be the source of truth
Theoretically yes, this is higher-level validation to provide a useful error early rather than punting it to later at the simulation/execution layer. It was actually requested/suggested by the community to implement this here: Discord thread,
Unified Address supporting all possible types (including claimable balances and liquidity pools) looks like a better idea to me. Can we just add the verification on the SDK level whether the particular address is supported for the contract calls (maybe inside TransactionBuilder)? This way, all address types will be supported everywhere, but once someone tries to use inappropriate address in a smart contract call, this will result in an error.
I think it's a little overzealous since as you said it will be caught later, but it does provide a better devx experience to see it prevented early. I think if someone truly wants to do this, they'd file an issue and demonstrate a real need at which point we'd do the whole "okay but why?" back and forth.
821c858 to
914e264
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, nice work, thanks for addressing feedback questions.
* Add strkey support for liquidity pools and claimable balances (#799) * Regenerate XDR with latest Protocol 23 version (#800) * Migrate cryptographic libraries to use modern @noble/curves. (#802) * Add support for encoding `Address` to/from new `StrKey`s (#801) * Allow specifying mixed types for vectors in `nativeToScVal` (#803) * Prepare v14.0.0-rc.1, Protocol 23 for release (#804)