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

Add the new field Expiry. #55

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GylmarGonzalez
Copy link

@GylmarGonzalez GylmarGonzalez commented Dec 15, 2024

Modify :
M migration/src/m20230608_071249_init_db.rs
M src/wallet/offline.rs

I only add the new field.

The files that were modified after the creation of the database, their changes were reverted with git

When I try to build the app I obtain this error.
error[E0609]: no field ExactExpiry on type database::TransferData
--> src/wallet/offline.rs:1034:60
|
1034 | expiration: td.expiration.map(|e| Some((e, td.ExactExpiry.unwrap()))),
| ^^^^^^^^^^^ unknown field
|
= note: available fields are: kind, status, batch_transfer_idx, txid, receive_utxo ... and 4 others

@zoedberg
Copy link
Collaborator

@GylmarGonzalez There's no need to open new PRs every time there's some code you need to change, it creates spam and confusion. Please go back to what was already clarified in #54 and check the provided info again.

@GylmarGonzalez
Copy link
Author

@GylmarGonzalez There's no need to open new PRs every time there's some code you need to change, it creates spam and confusion. Please go back to what was already clarified in #54 and check the provided info again.

Hi the #54 its closed.

Its my two last commit, was here in this PR.
GylmarGonzalez@086ba71
GylmarGonzalez@b91302d

@zoedberg
Copy link
Collaborator

Hi the #54 its closed.

Still please don't open new PRs from now on, let's use this.

Its my two last commit, was here in this PR.

Not sure what you are trying to say. Anyway I still don't see the changes that I described in #54 (comment) (the src/database/entities/batch_transfer.rs should have 3 lines of diff)

@GylmarGonzalez
Copy link
Author

Hi the #54 its closed.

Still please don't open new PRs from now on, let's use this.

Its my two last commit, was here in this PR.

Not sure what you are trying to say. Anyway I still don't see the changes that I described in #54 (comment) (the src/database/entities/batch_transfer.rs should have 3 lines of diff)

Ok I used this PR, I check the observation. Thanks.

@GylmarGonzalez
Copy link
Author

Hi.

I see the instruccion I modify the file src/database/entities/batch_transfer.rs

GylmarGonzalez@f34963e

@zoedberg
Copy link
Collaborator

I see the instruccion I modify the file src/database/entities/batch_transfer.rs

Great. Now you need to edit the code until it builds, to do that you will need to implement the feature described in #37

@GylmarGonzalez
Copy link
Author

ok,thank you for your patience.

I see the instruction on :
#38

Exactly this text:

Tests should be updated as following:
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

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
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)

I have a question , this explanation it for test the change after to do the change in code ? or this explanation it for do the change in code ?

Thanks.

@zoedberg
Copy link
Collaborator

zoedberg commented Dec 17, 2024

I have a question , this explanation it for test the change after to do the change in code ? or this explanation it for do the change in code ?

This explanation is to make existing tests build and to add new ones to test the new functionality. For now you can just try to make the code build again and then move on to the tests (unless you want to use a TDD approach).

@GylmarGonzalez
Copy link
Author

I have a question , this explanation it for test the change after to do the change in code ? or this explanation it for do the change in code ?

This explanation is to make existing tests build and to add new ones to test the new functionality. For now you can just try to make the code build again and then move on to the tests.

Understood, do you have any documentation or any idea about the change because the original explanation is general,

blind_receive and witness_receive I need agregate the bool parameter, I found the funtion in many file, Could you tell me how to proceed?

Thanks.

@zoedberg
Copy link
Collaborator

Understood, do you have any documentation or any idea about the change because the original explanation is general,

The original explanation is not that general, it assumes that the developer has familiarity with Rust.

blind_receive and witness_receive I need agregate the bool parameter, I found the funtion in many file, Could you tell me how to proceed?

You need to start from the public functions. You will find those functions calls in several files but their definitions in a single file (src/wallet/offline.rs).

@GylmarGonzalez
Copy link
Author

Understood, do you have any documentation or any idea about the change because the original explanation is general,

The original explanation is not that general, it assumes that the developer has familiarity with Rust.

blind_receive and witness_receive I need agregate the bool parameter, I found the funtion in many file, Could you tell me how to proceed?

You need to start from the public functions. You will find those functions calls in several files but their definitions in a single file (src/wallet/offline.rs).

In my case, I have never worked with Rust, that is why my learning curve is difficult,but I will try to fix it

I Read the explanation and I have a few question.

Let's add the exact_expiry bool parameter to receive APIs (blind_receive and witness_receive) that determines how an expired transfer should be handled.

When true, an expired transfer should be rejected.
When false, an expired transfer should be accepted anyway.

I take your recommendation, start from the public functions (src/wallet/offline.rs) I located the funtions (blind_receive and witness_receive), but
I dont understand when is a transfer expired? or how can validate a transfer expired.

I understand I save some information in database, expecificaly the new field, the action would be a insert or update ? I know I will use sea orm to do this task.
And I dont know the value for this Integer pub expiration: Option<(i64, bool)> the value bool yea I know the true or false the validation for expired transfer.

I have not found this method _handle_expired_transfers.

I understand this API fail_transfers I must change with the same process from the previous point.

I appreciate that you can clarify my doubts

@zoedberg
Copy link
Collaborator

I take your recommendation, start from the public functions (src/wallet/offline.rs) I located the funtions (blind_receive and witness_receive), but I dont understand when is a transfer expired? or how can validate a transfer expired.

A transfer is expired when it's still in the WaitingCounterparty status and its expiration time, if present, is smaller than the current time.

I understand I save some information in database, expecificaly the new field, the action would be a insert or update ? I know I will use sea orm to do this task.

At insert time. When blind_receive and witness_receive create a new batch transfer you will need to set the value for the exact expiry.

And I dont know the value for this Integer pub expiration: Option<(i64, bool)> the value bool yea I know the true or false the validation for expired transfer.

This will contain the expiration time (which is already calculated as now + duration_seconds) and the exact_expiry value.

I have not found this method _handle_expired_transfers.

This method has been removed. The logic has been moved to the fail_transfers method:

let now = now().unix_timestamp();
let mut expired_batch_transfers: Vec<DbBatchTransfer> = db_data
.batch_transfers
.clone()
.into_iter()
.filter(|t| t.waiting_counterparty() && t.expiration.unwrap_or(now) < now)
.collect();
for batch_transfer in expired_batch_transfers.iter_mut() {
if no_asset_only {
let connected_assets = batch_transfer
.get_asset_transfers(&db_data.asset_transfers)?
.iter()
.any(|t| t.asset_id.is_some());
if connected_assets {
continue;
}
}
transfers_changed = true;
self._try_fail_batch_transfer(batch_transfer, false, &mut db_data)?

@GylmarGonzalez
Copy link
Author

ok , I see you last comment, and I try the modify the method blind_receive src/wallet/offline.rs.

`
pub fn blind_receive(
&self,
asset_id: Option,
amount: Option,
duration_seconds: Option,
transport_endpoints: Vec,
min_confirmations: u8,
) -> Result<ReceiveData, Error> {
info!(
self.logger,
"Receiving via blinded UTXO for asset '{:?}' with duration '{:?}'...",
asset_id,
duration_seconds
);

    let mut unspents: Vec<LocalUnspent> = self.database.get_rgb_allocations(
        self.database.get_unspent_txos(vec![])?,
        None,
        None,
        None,
    )?;
    unspents.retain(|u| {
        !(u.rgb_allocations
            .iter()
            .any(|a| !a.incoming && a.status.waiting_counterparty()))
    });
    let utxo = self.get_utxo(vec![], Some(unspents), true)?;
    debug!(
        self.logger,
        "Blinding outpoint '{}'",
        utxo.outpoint().to_string()
    );
    let blind_seal =
        BlindSeal::opret_first_rand(BpTxid::from_str(&utxo.txid).unwrap(), utxo.vout);
    let graph_seal = GraphSeal::from(blind_seal);
    let seal = XChain::with(Layer1::Bitcoin, graph_seal);
    let beneficiary = Beneficiary::BlindedSeal(graph_seal.conceal());

    let (recipient_id, invoice, expiration_timestamp, batch_transfer_idx, asset_transfer_idx) =
        self._receive(
            asset_id,
            amount,
            duration_seconds,
            transport_endpoints,
            min_confirmations,
            beneficiary,
            RecipientType::Blind,
        )?;

    let mut runtime = self.rgb_runtime()?;
    runtime.store_secret_seal(seal)?;

    let db_coloring = DbColoringActMod {
        txo_idx: ActiveValue::Set(utxo.idx),
        asset_transfer_idx: ActiveValue::Set(asset_transfer_idx),
        r#type: ActiveValue::Set(ColoringType::Receive),
        amount: ActiveValue::Set(s!("0")),
        ..Default::default()
    };
    self.database.set_coloring(db_coloring)?;

    self.update_backup_info(false)?;

            // issue 37
    // blind_receive and witness_receive create a new batch transfer 
    let now = now().unix_timestamp(); 
    // get conexion to database
    let mut db_data = self.database.get_db_data(false)?;
    // get object batch transfer by id
    let batch_transfer = &self
    .database
    .get_batch_transfer_or_fail(batch_transfer_idx, &db_data.batch_transfers)?;
    // check the status to WaitingCounterparty
    if batch_transfer.waiting_counterparty() {
        // get exact_expiry
        let exact_expiry_v = now + duration_seconds;
        // if present, is smaller than the current time.
        if exact_expiry_v < now {
            batch_transfer.expiration = exact_expiry_v;
            batch_transfer.exact_expiry = "true";
        }else {
            batch_transfer.expiration = exact_expiry_v;
            batch_transfer.exact_expiry = "false";
        }

        // update the batch_transfer
        batch_transfer.update(db_data).await?;
    }

    info!(self.logger, "Blind receive completed");
    Ok(ReceiveData {
        invoice,
        recipient_id,
        expiration_timestamp,
        batch_transfer_idx,
    })

}

`

But I don't know if I'm doing it correctly.

@zoedberg
Copy link
Collaborator

But I don't know if I'm doing it correctly.

No. Please re-read carefully the issue description and my previous messages. The expiration is already implemented, you just need to add an exact_expiry boolean field to the receive APIs method signatures and save that value in the DB. Then on the fail_transfers API you need to read that value to change the behavior of the code that puts expired transfers in a Failed status.

Also please when you need feedback on code changes push them instead of writing them in a message.

@GylmarGonzalez
Copy link
Author

But I don't know if I'm doing it correctly.

No. Please re-read carefully the issue description and my previous messages. The expiration is already implemented, you just need to add an exact_expiry boolean field to the receive APIs method signatures and save that value in the DB. Then on the fail_transfers API you need to read that value to change the behavior of the code that puts expired transfers in a Failed status.

Also please when you need feedback on code changes push them instead of writing them in a message.

Yes, thanks for the comment and for all your help. I will try to send the idea of ​​the change in a commit. I hope to make my last attempt tonight (San Salvador time). Tomorrow, December 20th, is the deadline.

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 this pull request may close these issues.

2 participants