Testing exact expiry #38

arkanoider · 2023-12-05T17:03:35Z

Hi,

pardon my noobness first of all.

Was trying to learn and contribute: this is what could be a starting point for issue #37

Let me know how far I am from the right idea.

zoedberg · 2023-12-06T17:29:12Z

Hi! Thanks for your contribution.

The issue we wrote was not super clear, sorry for that. We said that the exact_expiry boolean should be addded to the blind_receive and witness_receive methods, which is correct, but we forgot to say that the boolean should then be saved to the BatchTransfer DB object. Adding a field to a DB object requires a minimal knowledge of how sea-orm tools work. To ease this we added an explanation on the migrations sub-crate README (https://github.com/RGB-Tools/rgb-lib/blob/next/migration/README.md).

With that field in place the _handle_expired_transfers method will not get an exact_expiry boolean from the method params but instead from each transfer object from the expired_transfers vector.

Some notes:

I've seen a variable named expired_order, we never refer to transfers as orders, so could you please rename that to expired_batch_transfer?
The Transfer object should be edited as:

pub struct Transfer {
    /// ID of the transfer
    pub idx: i32,
    /// Timestamp of the transfer creation
    pub created_at: i64,
    /// Timestamp of the transfer last update
    pub updated_at: i64,
    /// Status of the transfer
    pub status: TransferStatus,
    /// Amount in RGB unit (not considering precision)
    pub amount: u64,
    /// Type of the transfer
    pub kind: TransferKind,
    /// ID of the Bitcoin transaction anchoring the transfer
    pub txid: Option<String>,
    /// Recipient ID (blinded UTXO or Bitcoin script) of an incoming transfer
    pub recipient_id: Option<String>,
    /// UTXO of an incoming transfer
    pub receive_utxo: Option<Outpoint>,
    /// Change UTXO of an outgoing transfer
    pub change_utxo: Option<Outpoint>,
-   /// Expiration of the transfer
-   pub expiration: Option<i64>,
+   /// Expiration of the transfer and whether it should be exact
+   pub expiration: Option<(i64, bool)>,
    /// Transport endpoints for the transfer
    pub transport_endpoints: Vec<TransferTransportEndpoint>,
}

Tests should be updated as following:
1. update test_blind_receive and test_witness_receive methods (in src/wallet/test/utils/api.rs) in order to set the exact_expiry parameter to false
2. edit the blind_receive::success and witness_receive::success tests:
  - to check that exact_expiry is set to false (both on the batch transfer retrieved from the DB and on the transfer object returned by the list_transfers method)
  - to add a sub-test that calls the receive method with exact_expiry set to true, to then perform the previous checks again, this time checking it's set to true
3. add a new test called exact_expiry to the fail_tranfers and refresh test modules, that calls a receive method with a short expiration and exact_expiry set to true, waits for the transfer to expire (with a small sleep), sends the asset, and finally fails or refreshes the transfer to check that the transfer correctly goes to the Failed status (even if the transfer arrived)

If you find changing the DB too complex or don't want to install the necessary tools, don't worry, just let us know and we can prepare a commit with that change and you can continue the rest of the task on top of that commit. Also, since we are preparing a backwards incompatible release (0.3.0), instead of generating a new migration file, you could also change the existing one (migration/src/m20230608_071249_init_db.rs). It would be way easier since it would just require to add a col instruction to the BatchTransfer creation:

manager
    .create_table(
        Table::create()
            .table(BatchTransfer::Table)
            .if_not_exists()
        [...]
        .col(ColumnDef::new(BatchTransfer::Expiration).big_unsigned())
+       .col(ColumnDef::new(BatchTransfer::ExactExpiry).boolean())

and a field to the BatchTransfer enum:

#[derive(DeriveIden)]
pub enum BatchTransfer {
    Table,
    Idx,
    Txid,
    Status,
    CreatedAt,
    UpdatedAt,
    Expiration,
+   ExactExpiry,
    MinConfirmations,
}

By doing so you will not need to call the sea-orm-cli migrate generate command, but you'll still need to pull the postgres docker image and follow the rest of the README explanation from that on.

Note: as you see the new parameter is nullable, because when the expiration will be null the exact_expiry field should also be null, otherwise it should also be present.

arkanoider · 2023-12-06T21:31:33Z

Hi @zoedberg !

thanks for your time spent to answer to my noob tentative.

I supposed that I was missing some points, and you explained me really well what is the needing of the issue.

If no some smarter guy ( or you ) create the PR before I can dig a little bit inside your suggestion I will try to update my PR to get it merged. I am still at the beginning of my study or RGB and I stumbled upon these issues that seems quite affordable for me, also because of Rust.

I am working with another guy on Mostro and learning more about bitcoin and Rust, but I really think RGB is a real nice improvement for bitcoin in case of success...so learning a bit of it for me could be a great achievement!

Hope I can update this PR with your suggestions asap!

zoedberg · 2023-12-07T09:53:02Z

We are happy to wait for you. If you don't complete the task in time for the 0.3.0 don't worry, we'll do also a 0.4.0 once RGB 0.11 will be ready. Thanks again

arkanoider · 2023-12-07T10:41:42Z

We are happy to wait for you. If you don't complete the task in time for the 0.3.0 don't worry, we'll do also a 0.4.0 once RGB 0.11 will be ready. Thanks again

You're welcome! By the way I think that we are from the same region... ;)

arkanoider · 2023-12-19T18:20:20Z

Hi @zoedberg !

still alive...

I was starting back for my second shot using your precious advices....one thing...

pub struct Transfer {
    /// ID of the transfer
    pub idx: i32,
    /// Timestamp of the transfer creation
    pub created_at: i64,
    /// Timestamp of the transfer last update
    pub updated_at: i64,
    /// Status of the transfer
    pub status: TransferStatus,
    /// Amount in RGB unit (not considering precision)
    pub amount: u64,
    /// Type of the transfer
    pub kind: TransferKind,
    /// ID of the Bitcoin transaction anchoring the transfer
    pub txid: Option<String>,
    /// Recipient ID (blinded UTXO or Bitcoin script) of an incoming transfer
    pub recipient_id: Option<String>,
    /// UTXO of an incoming transfer
    pub receive_utxo: Option<Outpoint>,
    /// Change UTXO of an outgoing transfer
    pub change_utxo: Option<Outpoint>,
    /// Expiration of the transfer
    pub expiration: Option<i64>, 
    /// Expiration of the transfer and whether it should be exact
    pub expiration: Option<(i64, bool)>,   // Added bool here!
    /// Transport endpoints for the transfer
    pub transport_endpoints: Vec<TransferTransportEndpoint>,
}

This is crystal clear, but...after compiler error this lead to a modification to this struct below:

#[derive(Debug)]
pub(crate) struct TransferData {
    pub(crate) kind: TransferKind,
    pub(crate) status: TransferStatus,
    pub(crate) txid: Option<String>,
    pub(crate) receive_utxo: Option<Outpoint>,
    pub(crate) change_utxo: Option<Outpoint>,
    pub(crate) created_at: i64,
    pub(crate) updated_at: i64,
    pub(crate) expiration: Option<(i64, bool)>,  // Added bool here!
}

and this lead to modify this...

#[derive(Clone, Debug, PartialEq, DeriveModel, DeriveActiveModel, Eq)]
pub struct Model {
    pub idx: i32,
    pub txid: Option<String>,
    pub status: TransferStatus,
    pub created_at: i64,
    pub updated_at: i64,
    pub expiration: Option<(i64, bool)>, // Added bool here!
    pub min_confirmations: u8,
}

Resulting in Rust telling me this...

....
error[E0277]: the trait bound `sea_orm::Value: From<(i64, bool)>` is not satisfied
  --> src/database/entities/batch_transfer.rs:16:48
   |
16 | #[derive(Clone, Debug, PartialEq, DeriveModel, DeriveActiveModel, Eq)]
   |                                                ^^^^^^^^^^^^^^^^^ the trait `From<(i64, bool)>` is not implemented for `sea_orm::Value`
   |
   = help: the following other types implement trait `From<T>`:
             <sea_orm::Value as From<&[u8]>>
             <sea_orm::Value as From<&std::string::String>>
             <sea_orm::Value as From<&str>>
             <sea_orm::Value as From<Cow<'_, str>>>
             <sea_orm::Value as From<JsonValue>>
             <sea_orm::Value as From<OffsetDateTime>>
             <sea_orm::Value as From<PrimitiveDateTime>>
             <sea_orm::Value as From<Vec<T>>>
           and 30 others
   = note: required for `(i64, bool)` to implement `Into<sea_orm::Value>`
   = note: required for `sea_orm::Value` to implement `From<std::option::Option<(i64, bool)>>`
   = note: 1 redundant requirement hidden
   = note: required for `std::option::Option<(i64, bool)>` to implement `Into<sea_orm::Value>`
note: required by a bound in `ActiveValue`
  --> /home/lupin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sea-orm-0.12.8/src/entity/active_model.rs:41:8
   |
39 | pub enum ActiveValue<V>
   |          ----------- required by a bound in this enum
40 | where
41 |     V: Into<Value>,
   |        ^^^^^^^^^^^ required by this bound in `ActiveValue`
   = note: this error originates in the derive macro `DeriveActiveModel` (in Nightly builds, run with -Z macro-backtrace for more info)

In my undestanding there is a missing From Trait implementation missing in sea-orm for tuples. Really far from me to touch sea orm lib and so if I undestand the compiler issue correctly could we think about to split the tuple in two vars like this...

pub struct Transfer {
    /// ID of the transfer
    pub idx: i32,
    /// Timestamp of the transfer creation
    pub created_at: i64,
    /// Timestamp of the transfer last update
    pub updated_at: i64,
    /// Status of the transfer
    pub status: TransferStatus,
    /// Amount in RGB unit (not considering precision)
    pub amount: u64,
    /// Type of the transfer
    pub kind: TransferKind,
    /// ID of the Bitcoin transaction anchoring the transfer
    pub txid: Option<String>,
    /// Recipient ID (blinded UTXO or Bitcoin script) of an incoming transfer
    pub recipient_id: Option<String>,
    /// UTXO of an incoming transfer
    pub receive_utxo: Option<Outpoint>,
    /// Change UTXO of an outgoing transfer
    pub change_utxo: Option<Outpoint>,
    /// Expiration of the transfer
    pub expiration: Option<i64>,
    /// Expiration of the transfer and whether it should be exact      
    pub expiration: Option<i64>,
    pub exact_expiry: Option<bool>,   // Added bool here!
    /// Transport endpoints for the transfer
    pub transport_endpoints: Vec<TransferTransportEndpoint>,
}

This should make Rust happy, but as I told you sorry if am not on the right path...

Waiting for your opinion!

zoedberg · 2023-12-20T13:34:06Z

Hi @arkanoider!

You shouldn't manually edit the Model struct, that struct needs to be edited by sea-orm-cli, please check instructions in https://github.com/RGB-Tools/rgb-lib/blob/master/migration/README.md to learn how to do that.

By editing the migration/src/m20230608_071249_init_db.rs file as I previously suggested (the extra col call to the create_table instruction and the extra ExactExpiry field to the BatchTransfer enum) and running the appropriate sea-orm-cli commands you will end up having an updated Model for the batch transfer.
Also, note that it's not possible to have a model with a field like the one you are trying to define:

pub expiration: Option<(i64, bool)>, // Added bool here!

Because databases don't support tuples.
By using the code I've suggested, sea-orm-cli will add an exact_expiry boolean field to the model. While all the other structs that are not reflecting the DB structures, like Transfer and TransferData, can instead have the expiration field defined as a tuple.

I hope you'll find my explanation helpful and clear enough, otherwise don't hesitate to ask for more clarification.

arkanoider · 2023-12-20T15:24:02Z

Hi @arkanoider!

You shouldn't manually edit the Model struct, that struct needs to be edited by sea-orm-cli, please check instructions in https://github.com/RGB-Tools/rgb-lib/blob/master/migration/README.md to learn how to do that.

By editing the migration/src/m20230608_071249_init_db.rs file as I previously suggested (the extra col call to the create_table instruction and the extra ExactExpiry field to the BatchTransfer enum) and running the appropriate sea-orm-cli commands you will end up having an updated Model for the batch transfer. Also, note that it's not possible to have a model with a field like the one you are trying to define:
pub expiration: Option<(i64, bool)>, // Added bool here!
Because databases don't support tuples. By using the code I've suggested, sea-orm-cli will add an exact_expiry boolean field to the model. While all the other structs that are not reflecting the DB structures, like Transfer and TransferData, can instead have the expiration field defined as a tuple.

I hope you'll find my explanation helpful and clear enough, otherwise don't hesitate to ask for more clarification.

Yep! Sounds clear...will try another shot asap.

arkanoider · 2023-12-24T11:48:09Z

Hi @zoedberg ,

going on in my spare time...first of all have a nice Xmas!! 🎄 🎅

Will let one of my thought here while reading RGB specs and trying to learn more.
Regards this:

pub expiration: Option<(i64, bool)>

correlated to this the Model struct updated:

pub struct Model {
    pub idx: i32,
    pub txid: Option<String>,
    pub status: TransferStatus,
    pub created_at: i64,
    pub updated_at: i64,
    pub expiration: Option<i64>,
    pub exact_expiry: Option<bool>,
    pub min_confirmations: u8,
}

A doubt about a cosmetics, if we use the tuple in Transfer struct like Option<(a , b)> then in someway we have to convert two Options of new Model struct like this:

( Option (a), Option (b) ) to something like Option( a, b ) which involves some logic. Could it be ok to create the tuple with 2 Options inside like this to avoid a single remapping of this kind?

pub expiration: (Option<i64>, Option<bool>)

let me know your thoughts...

zoedberg · 2024-01-08T14:08:01Z

Hi @arkanoider,
sorry for the late response, I was on holiday.

Could it be ok to create the tuple with 2 Options inside like this to avoid a single remapping of this kind?

I assume you are referring to the expiration field of the Transfer struct defined in the src/wallet/mod.rs file and not the field of the Model struct defined in the src/database/entities/batch_transfer.rs file (since this, as already said, is reflecting a DB structure and therefore cannot have tuple fields).

In the first answer I gave to this PR, I've asked to change the expiration field of the Transfer struct as the following:

-   /// Expiration of the transfer
-   pub expiration: Option<i64>,
+   /// Expiration of the transfer and whether it should be exact
+   pub expiration: Option<(i64, bool)>,

If I understood correctly you are asking if it's possible to change that field like so:

-   /// Expiration of the transfer
-   pub expiration: Option<i64>,
+   /// Expiration of the transfer and whether it should be exact
+   pub expiration: (Option<i64>, Option<bool>),

If that's the proposal, I don't agree with it, because, as I previosly said:

Note: as you see the new parameter is nullable, because when the expiration will be null the exact_expiry field should also be null, otherwise it should also be present.

This means that it's impossible to have the exact_expiry field set to Some and the expiration field set to None (combination that is instead possible with the field definition you are proposing).

The only possible combinations are:

we have an expiration therefore the exact_expiry will be false or true
we don't have an expiration, therefore the exact_expiry must be None too

The conversion from the 2 structures (Model and Transfer), performed in the from_db_transfer method, should therefore change as the following:

-    expiration: td.expiration,
+    expiration: td.expiration.map(|e| Some((e, tx.exact_expiry.unwrap()))),

It will be the developer's reponsibility to never insert into the DB a transfer that has an expiration set and an exact_expiry unset.

arkanoider · 2024-01-09T17:52:39Z

Hì @zoedberg ,

hope you enjoyed your holiday and had a nice start for 2024!

Yes in my mind i was thinkin what you correctly understood and it's clear why is not the right solution.
I had some offline days too in this last week and I will look back to the code in these...
Thanks for your answers!

zoedberg · 2024-02-22T14:42:05Z

Hi @arkanoider,
hope you're doing ok. I was wondering, are you still working on this?

arkanoider · 2024-02-23T10:16:38Z

Hi @arkanoider, hope you're doing ok. I was wondering, are you still working on this?

Hi @zoedberg sorry for my silence, i had a bad 2024 start for personal issues. I'd like to go on for sure, but I will have my mind back on track i need some more time...

zoedberg · 2024-02-23T14:37:36Z

Hi @zoedberg sorry for my silence, i had a bad 2024 start for personal issues. I'd like to go on for sure, but I will have my mind back on track i need some more time...

Sorry to hear that, hope things will get better. Currently we're still waiting for RGB 0.11 to be released, so we're not in a rush. In case we're about to release and this is still incomplete we'll probably make the DB changes and leave you all the rest of the work.

arkanoider · 2024-03-26T20:43:31Z

Hi @zoedberg ,

if the issue is still open I can try to spend some spare time to retry closing it! Personal issue are going better and also the other lightning project let me some time to study...meanwhile I am still trying to find my way in a bitcoin fulltime job!
Let me know if I can try to move on on this or you're going to close...

zoedberg · 2024-03-27T00:18:38Z

Hi @arkanoider! I'm happy to hear that things are going better for you! I think you'll have the time to tackle this

arkanoider · 2024-03-27T05:57:34Z

Thanks @zoedberg , will rewind my mind on it reading bsck the conversation with and will try to implement something!

Lucio Pinese added 2 commits December 5, 2023 17:47

first tentative of exact expiry

6129bcd

small fix on mut parameter

c71580d

arkanoider closed this by deleting the head repository Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing exact expiry #38

Testing exact expiry #38

arkanoider commented Dec 5, 2023

zoedberg commented Dec 6, 2023

arkanoider commented Dec 6, 2023

zoedberg commented Dec 7, 2023

arkanoider commented Dec 7, 2023

arkanoider commented Dec 19, 2023

zoedberg commented Dec 20, 2023

arkanoider commented Dec 20, 2023

arkanoider commented Dec 24, 2023 •

edited

Loading

zoedberg commented Jan 8, 2024 •

edited

Loading

arkanoider commented Jan 9, 2024

zoedberg commented Feb 22, 2024

arkanoider commented Feb 23, 2024

zoedberg commented Feb 23, 2024

arkanoider commented Mar 26, 2024

zoedberg commented Mar 27, 2024

arkanoider commented Mar 27, 2024

Testing exact expiry #38

Testing exact expiry #38

Conversation

arkanoider commented Dec 5, 2023

zoedberg commented Dec 6, 2023

arkanoider commented Dec 6, 2023

zoedberg commented Dec 7, 2023

arkanoider commented Dec 7, 2023

arkanoider commented Dec 19, 2023

zoedberg commented Dec 20, 2023

arkanoider commented Dec 20, 2023

arkanoider commented Dec 24, 2023 • edited Loading

zoedberg commented Jan 8, 2024 • edited Loading

arkanoider commented Jan 9, 2024

zoedberg commented Feb 22, 2024

arkanoider commented Feb 23, 2024

zoedberg commented Feb 23, 2024

arkanoider commented Mar 26, 2024

zoedberg commented Mar 27, 2024

arkanoider commented Mar 27, 2024

arkanoider commented Dec 24, 2023 •

edited

Loading

zoedberg commented Jan 8, 2024 •

edited

Loading