Wrap RET Addresses #10

CyonAlexRDX · 2024-02-20T18:27:46Z

I've removed the huge Sargon.swift file and gitignored it since I believe we don't need it checked in to git, see gitignore for rationale.

This PR vendors wrapping of RET's new Canonical*Address types, specifically:

AccountAddress (changed from existing to use new wrapping solution)
IdentityAddress (changed from existing to use new wrapping solution)
ResourceAddress (changed from existing to use new wrapping solution)
VaultAddress
ValidatorAddress
AccesscontrollerAddress
PoolAddress
PackageAddress
ComponentAddress

Also simplify how Decimal192 is implemented, removing one level of indirection.

All addresses have been fully Swiftified on the Swift side, including preview values.

The "interface" of ALL address is this (swift version)

public protocol AddressProtocol: Sendable, CustomStringConvertible {
	init(validatingAddress bech32String: String) throws
	var networkID: NetworkID { get }
	var address: String { get }
}

…mple app

…requires more fixed size BagOfBytes, introducing: Hex64Bytes, Hex65Bytes, Hex33Bytes, generated with macro.

…Key, and same for Secp256k1.

…ove sccache ref in direnv, it did not speed up things.

… in Package.swift, also required for when making releses.

…e, we can make external types such as Scrypto's Decimal Uniffi compat directly contrary to what I initially believed.

…moving the Inner type since it was not needed.

Sajjon · 2024-02-21T13:30:02Z

apple/Sources/Sargon/Extensions/Methods/Addresses+Wrap+Functions.swift

CyonAlexRDX · 2024-02-21T14:23:56Z

apple/Sources/UniFFI/Sargon.swift

I realize we might actually git rm --cached Sargon.swift this and then gitignore it. Because it is added part of all releases, and will be present locally...

CyonAlexRDX · 2024-02-21T14:24:46Z

src/core/types/decimal192.rs

-    Hash,
-    derive_more::Display,
-)]
-pub struct InnerDecimal(pub(crate) ScryptoDecimal192);


I had this because I (incorrectly) thought we could not add custom UniFFI conversion for external types, but we can, so no need for this level of indirection.

CyonAlexRDX · 2024-02-21T14:24:58Z

src/core/types/decimal192.rs

@@ -88,7 +59,7 @@ pub struct Decimal192 {
    /// due to limitations in UniFII as of Feb 2024, but you should
    /// create extension methods on Decimal192 in FFI land, translating
    /// these functions into methods.)
-    __inner: InnerDecimal, // Strange field name to try as much as possible hide it in FFI land.


UniFFI did in fact remove __ prefix...

CyonAlexRDX · 2024-02-21T14:27:22Z

src/profile/v100/address/accesscontroller_address.rs

+    uniffi::Record,
+)]
+#[display("{secret_magic}")]
+pub struct AccessControllerAddress {


Declare "manually" and NOT by macro so we can add documentation to it! which is not possible with macro_rules! (or at least too hard for me...).

Ah wrong! we CAN include doc strings in macros, change incoming.

CyonAlexRDX · 2024-02-21T14:28:55Z

src/profile/v100/address/wrap_ret_address.rs

+    ) -> Result<Self>;
+}
+
+macro_rules! decl_ret_wrapped_address {


This macro creates the needed UniFFI converter for each RET address, using String as Builtin type. It impl From the wrapped RET address type into the Sargon address type. and all UniFFI exported function

CyonAlexRDX · 2024-02-21T14:29:57Z

src/profile/v100/address/wrap_ret_address.rs

+    RetAccessControllerAddress,
+    accesscontroller
+);
+decl_ret_wrapped_address!(AccountAddress, RetAccountAddress, account);


N.B. the actual Sargon address types, e.g. AccountAddress, PackageAddress etc are defined in other files, outside of the macro, so that we can add documentation to them, which is not possible to "inject" into macros.

Ah wrong! we CAN include doc strings in macros, change incoming.

CyonAlexRDX · 2024-02-21T14:34:21Z

src/sargon.udl

unfortunate but, c'est la vie, declaring the Ret canonical address as UniFFI convertible via String make implementation of our wrapping of these types easy and allows for safe Rust code, and safe Swift/Kotlin models, so best of two worlds, at the cost of having to declare them here in the UDL file...

GhenadieVP

Very very nice 👏!

GhenadieVP · 2024-02-21T15:23:05Z

_typos.toml

@@ -9,6 +9,7 @@ extend-exclude = [

 [default.extend-identifiers]
 inout = "inout"
+pool_tdx_2_1c4ml86h8lvfk7jma0jy0vksh8srcxhmtax8nd3aur29qtd2k2wmlzk = "pool_tdx_2_1c4ml86h8lvfk7jma0jy0vksh8srcxhmtax8nd3aur29qtd2k2wmlzk"


what is this for?

Typos CLI tool corrected that address 😅😅 so had to ignore it

GhenadieVP · 2024-02-21T15:29:51Z

src/profile/v100/entity/abstract_entity_type.rs

@@ -19,37 +19,8 @@ use radix_engine_common::types::EntityType as ScryptoEntityType;
 )]
 #[repr(u32)] // it is u32 since used in Derivation Paths (CAP26) where each component is a u32.
 pub enum AbstractEntityType {


So this is meant only to describe an Entity in the sense of either Account or Identity, but not other address kind?

Yes, account or identity only. I can see if I can remove this perhaps

GhenadieVP · 2024-02-21T15:34:49Z

apple/Sources/Sargon/Extensions/Methods/Addresses+Wrap+Functions.swift

+
+#if DEBUG
+	/// TODO: Declare in Rust land? Make non-DEBUG?
+	public enum Address: Hashable, Equatable, Sendable {


This will most probably be needed in the iOS Wallet, where we do need to merge together different kinds of addresses in some places.

Will be added in Rust land immediately when I see we really need it. I still - naive perhaps - think we can avoid having it in wallets. Or at least a smaller tagged union (fewer cases)

Sajjon · 2024-02-21T19:34:35Z

UniFFI macro invocations introduces a lot of false negatives, decreasing the accuracy in reported code coverage, I've asked about if we can improve accuracy in xd009642/tarpaulin#351 (comment)

Will merge this PR as is, nothing we can do.

Increase code coverage further

Sajjon and others added 30 commits February 13, 2024 20:50

Arithmetic operations for Decimal

eedf6a2

Decimal compare methods and UniFFI exported funtions

d4a019a

add Swift bindgen tests for decimal

ed6e18e

Merge branch 'develop' into wrap_radix_engine_toolkit

2e95bfb

Sargon: Add method to read Mnemonic, UniFFI exported, Improve iOS exa…

30f134d

…mple app

Merge

2c447c5

WIP Decimal192 + Swifitfied

944795e

Make Decimal192 conform to Comparable

a9b96d0

Add clamped

5ec79b6

Decimal rounding

ff8a4a4

rename file decimal -> decimal192.rs, add from f32.

ac0e057

Add from_formatted_string

3dc5907

Swiftify Decimal192 from formatted string

b2c90d9

Add tests

47f6da9

more tests

7592b04

Start stubbing out Signatures and other types required by RET, which …

9437497

…requires more fixed size BagOfBytes, introducing: Hex64Bytes, Hex65Bytes, Hex33Bytes, generated with macro.

Declare Ed25519Signature and use it with Ed25519PrivateKey and Public…

5f5cc20

…Key, and same for Secp256k1.

drop Ord/PartialOrd for types where it did not really make sense. Rem…

fe86e8c

…ove sccache ref in direnv, it did not speed up things.

Add HasPlaceholder conformance to Ed25519PublicKey

9aad4ff

Add HasPlaceholder conformance to Secp256k1Signature

b31d4fa

tests of Ed25519Signature and Secp256k1Signature

1731692

Unit tests for Signature

3d286fc

Merge branch 'develop' into wrap_radix_engine_toolkit

6a7efd4

Add step to precommit ensuring that local development is set to false…

95931b9

… in Package.swift, also required for when making releses.

fail fast in precommit

32e15fb

build, here is the updated Swift bindgen

edd3748

address WIP

526e887

WIP wrap RET addresses

c21494b

WIP wrapping addresses with macro

0623404

WIP addresses

24f5327

Sajjon added 4 commits February 21, 2024 09:19

simplify Decimal192, there was in fact no need for a InnerDecimal typ…

8075d47

…e, we can make external types such as Scrypto's Decimal Uniffi compat directly contrary to what I initially believed.

Document ALL Address types - Simplify wrapping of RET Addresses by re…

535f5b4

…moving the Inner type since it was not needed.

Swiftify all address types

cdd598f

more tests

4ca2f3d

Sajjon reviewed Feb 21, 2024

View reviewed changes

apple/Sources/Sargon/Extensions/Methods/Addresses+Wrap+Functions.swift

Copy link

Contributor

Sajjon Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

revert Vstack

e431827

CyonAlexRDX commented Feb 21, 2024

View reviewed changes

CyonAlexRDX requested review from micbakos-rdx and GhenadieVP February 21, 2024 14:25

CyonAlexRDX commented Feb 21, 2024

View reviewed changes

simplify addresses

7177afa

GhenadieVP approved these changes Feb 21, 2024

View reviewed changes

CyonAlexRDX changed the title ~~Wrap ret 2 addresses~~ Wrap RET Addresses Feb 21, 2024

Sajjon added 8 commits February 21, 2024 19:05

moar tests

2a6a34a

codecov accuracy

9c577a9

Oops, Forgot to use the doc string for address types, now fixed.

d167ab9

Test debug of Addresses

1b9f7d1

git rm and gitignore apple/Sources/UniFFI/Sargon.swift

74352b9

Moar tests

4080391

try increase accuracy

43e257c

moar tests

f606f4c

CyonAlexRDX merged commit eb0bc7d into develop Feb 21, 2024
10 of 12 checks passed

CyonAlexRDX deleted the wrap_ret_2_addresses branch February 21, 2024 19:34

CyonAlexRDX pushed a commit that referenced this pull request Apr 1, 2024

Merge pull request #10 from Sajjon/code_cov

d4bacab

Increase code coverage further

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap RET Addresses #10

Wrap RET Addresses #10

CyonAlexRDX commented Feb 20, 2024 •

edited

Loading

Sajjon Feb 21, 2024

CyonAlexRDX Feb 21, 2024

CyonAlexRDX Feb 21, 2024

CyonAlexRDX Feb 21, 2024

CyonAlexRDX Feb 21, 2024

CyonAlexRDX Feb 21, 2024

CyonAlexRDX Feb 21, 2024

CyonAlexRDX Feb 21, 2024

CyonAlexRDX Feb 21, 2024

CyonAlexRDX Feb 21, 2024

GhenadieVP left a comment

GhenadieVP Feb 21, 2024

CyonAlexRDX Feb 21, 2024

GhenadieVP Feb 21, 2024

CyonAlexRDX Feb 21, 2024

GhenadieVP Feb 21, 2024

CyonAlexRDX Feb 21, 2024

Sajjon commented Feb 21, 2024

Wrap RET Addresses #10

Wrap RET Addresses #10

Conversation

CyonAlexRDX commented Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GhenadieVP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sajjon commented Feb 21, 2024

CyonAlexRDX commented Feb 20, 2024 •

edited

Loading