chore(release): v1.0.7-beta #1936
Conversation
The file permissions of the cli config file is now set to 660 to disallow reading by other users.
Activation types in cli have been introduced by this commit to ensure that if a malicious person substituted them in the activation scheme file it would not lead to any unexpected action.
This commit fixes PoSV coins withdrawal issue. The issue was a missing n_time field in the generated transaction. The fix now correctly considers when n_time is required, and the rawtransaction can be broadcasted.
Signed-off-by: ozkanonur <[email protected]>
- This commmit fixes transactions that transfer multiple NFT tokens in db. These transactions cause errors when adding due to the db constraint on tx hash uniqueness. To solve this, the PR uses log_index as part of the transfers history table primary key. - nft_tx_history table is now called nft_transfer_history and tx/txs are renamed to transfer/transfers throughout the NFT code since what's added/retrieved from DB is NFT transfers not transactions (Multiple NFT transfers can be in one transaction). By renaming the table, there are no need for db migrations due to the addition of log_index column. Although NFT is not used in production yet, if anybody used it, transfers will be re-fetched and saved to the new DB table when using the related API methods.
shamardy
added
under review
and removed
in progress
| @@ -23,14 +25,14 @@ mm2_net = { path = "../mm2_net" } | |||
| mm2_number = { path = "../mm2_number" } | |||
| mm2_rpc = { path = "../mm2_rpc"} | |||
| passwords = "3.1" | |||
| rpc = { path = "../mm2_bitcoin/rpc" } | |||
| rustls = { version = "^0.20.4", features = [ "dangerous_configuration" ] } | |||
There was a problem hiding this comment.
@rozhkovdmitrii why two diff versions? (we seem using a total of 3 diff across codebase, 0.19.1, 0.20.4 and 0.20.8)
cli lockfile:
[[package]]
name = "rustls"
version = "0.19.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "35edb675feee39aec9c99fa5ff985081995a06d594114ae14cbe797ad7b7a6d7"
dependencies = [
"base64 0.13.1",
"log 0.4.17",
"ring",
"sct 0.6.1",
"webpki 0.21.4",
]
[[package]]
name = "rustls"
version = "0.20.8"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "fff78fc74d175294f4e83b28343315ffcfb114b156f0185e9741cb5570f50e2f"
dependencies = [
"log 0.4.17",
"ring",
"sct 0.7.0",
"webpki 0.22.0",
]
There was a problem hiding this comment.
jfyi: dangerous_configuration: this feature enables a dangerous() method on ClientConfig and ServerConfig that allows setting inadvisable options, such as replacing the certificate verification process. Applications requesting this feature should be reviewed carefully.
assume this is for self-signed / local cert handling? cc @shamardy
There was a problem hiding this comment.
assume this is for self-signed / local cert handling?
Yes. To disable certificate verification from cli side dangerous_configuration has to be used
komodo-defi-framework/mm2src/adex_cli/src/transport.rs
Lines 106 to 108 in 867a01a
P.S.
dangerous_configuration is not used from the https server side in mm2.
There was a problem hiding this comment.
Thank you for pointing it out )
Originally the version of rustls was not constrained and "0.20.8" was used in adex-cli.
Version "0.19.1" is used as subdependency of `mm2_net`
$ cargo tree --manifest-path mm2src/adex_cli/Cargo.toml -i [email protected]
rustls v0.19.1
├── adex-cli v0.1.0 (/home/rozhkov/sources/atomicDEX-API/mm2src/adex_cli)
└── futures-rustls v0.21.1
└── mm2_core v0.1.0 (/home/rozhkov/sources/atomicDEX-API/mm2src/mm2_core)
└── mm2_net v0.1.0 (/home/rozhkov/sources/atomicDEX-API/mm2src/mm2_net)
└── adex-cli v0.1.0 (/home/rozhkov/sources/atomicDEX-API/mm2src/adex_cli)On 03.08 I had to start using rustls as explicit dependency and I was oriented on using rustls 0.20.4. It was a version which of mm2 was dependent on. Perhaps I had to strongly tie adex-cli on 0.20.4 to be able to manage versions manually.
Now using both version "0.19.1" and "0.20.8" looks appropriate in my honest opinion
Concerning the question why dangerous_configuration feature was utilized - that was to make it able to connect to mm2 that issues self signed certificate.
Thank you
There was a problem hiding this comment.
Concerning the question why dangerous_configuration feature was utilized - that was to make it able to connect to mm2 that issues self signed certificate.
mm2 can be initialized using a certificate file too, it doesn't have to be self-signed but will be in most cases. It would be good to allow the cli user to disable certificate verification themselves like it's done in some other clients (e.g. postman provides this, it doesn't disable it by default). Please open an issue for this @rozhkovdmitrii and it can be done later as it's not urgent at all.
There was a problem hiding this comment.
Please open an issue for this @rozhkovdmitrii and it can be done later as it's not urgent at all.
There was a problem hiding this comment.
I just wanted to mention something about the versions of hyper-rustls and rustls. From what I understand, the version of rustls that we use should exactly match the version of rustls that hyper-rustls depends on. For example, if we are using hyper-rustls 0.23, we should also use rustls 0.20.8. This is important because if there is a version mismatch, such as using hyper-rustls 0.23 and rustls 0.21.7, we may encounter unexpected errors like below:
note: `ClientConfig` is defined in crate `rustls`
--> /home/decker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustls-0.21.7/src/client/client_conn.rs:128:1
|
128 | pub struct ClientConfig {
| ^^^^^^^^^^^^^^^^^^^^^^^
note: `rustls::client::client_conn::ClientConfig` is defined in crate `rustls`
--> /home/decker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustls-0.20.8/src/client/client_conn.rs:91:1
|
91 | pub struct ClientConfig {
| ^^^^^^^^^^^^^^^^^^^^^^^
= note: perhaps two different versions of crate `rustls` are being used?
To avoid any potential compatibility issues, it may be better to specify the exact versions of the crates using the = symbol. This way, we can ensure that the versions of hyper-rustls and rustls are precisely matched, reducing the chances of encountering any compatibility problems. Perhaps I may have slightly overestimated the significance of the "issue", but I have personally encountered package version mismatches during some of my own tests. Anyway, JFYI.
There was a problem hiding this comment.
Thank you for pointing it out 🙏, solved
There was a problem hiding this comment.
@rozhkovdmitrii can you please make this fix and the one here #1936 (comment) in a seperate PR? The whole release will be blocked until #1932 is sec reviewed and QA tested otherwise.
| rewrite_json_file(self, adex_path_str)?; | ||
| #[cfg(unix)] | ||
| { | ||
| set_file_permissions(adex_path_str, CFG_FILE_PERM_MODE)?; |
There was a problem hiding this comment.
is this advised? Wouldn't it be better/safer to assume config file has correct permission setting as opposed to implementing a de-facto "chmod" into mm2?
Rly concerned over likely unneeded filestream ops and speaking general - against this.
There was a problem hiding this comment.
I advised him to this as it's creating the config JSON that is used by a separate process, mm2.
There could be a better solution, but without this, the seed will be readable by any user on the system.
There was a problem hiding this comment.
my original comment
#1871 (comment)
There was a problem hiding this comment.
Yep, it was adviced.
The command: adex-cli config set -u http://localhost:77873 -p creates the configuration file and setting permissions could look quite essential.
cc: @Alrighttt
There was a problem hiding this comment.
It's not totally necessary. Maybe a warning while creating this configuration would suffice since the target audience of this app is presumably power users or at least users familiar with a terminal.
| } | ||
|
|
||
| #[cfg(unix)] | ||
| pub(crate) fn set_file_permissions(file: &str, unix_mode: u32) -> Result<()> { |
There was a problem hiding this comment.
duplicate feedback from above:
is this advised? Wouldn't it be better/safer to assume config file has correct permission setting as opposed to implementing a de-facto "chmod" into mm2?
Rly concerned over likely unneeded filestream ops and speaking general - against this.
SwapOpsV2 trait was added containing methods of the new protocol (WIP). SwapOpsV2 was implemented for UtxoStandardCoin. Dockerized integration tests added, sending and spending/refunding "dex fee + premium" UTXO. State machine was refactored as a preparation step for StorableStateMachine pattern extension. --------- Co-authored-by: Artem Vitae <[email protected]>
Activation scheme was changed hence related data types have to be fit for it.
Signed-off-by: ozkanonur <[email protected]>
| .push_opcode(Opcode::OP_2) | ||
| .push_bytes(pub_0) | ||
| .push_bytes(pub_1) | ||
| .push_opcode(Opcode::OP_2) |
There was a problem hiding this comment.
<pubkey0> OP_CHECKSIGVERIFY <pubkey1> OP_CHECKSIG is more efficient than OP_CHECKMULTISIG if it's used for only two keys
There was a problem hiding this comment.
Noted, I will do it in the next iteration(s).
| @@ -22,7 +22,7 @@ use std::sync::{Arc, Mutex}; | |||
|
|
|||
| fn nft_list_table_name(chain: &Chain) -> String { chain.to_ticker() + "_nft_list" } | |||
|
|
|||
| fn nft_tx_history_table_name(chain: &Chain) -> String { chain.to_ticker() + "_nft_tx_history" } | |||
| fn nft_transfer_history_table_name(chain: &Chain) -> String { chain.to_ticker() + "_nft_transfer_history" } | |||
There was a problem hiding this comment.
Any chain ticker starting with a number will cause the sql statement that creates the table to fail unexpectedly.
There was a problem hiding this comment.
I realize this isn't directly related to the changes in this PR. However, if we are still in the process of restructuring the DB, please consider #1916
There was a problem hiding this comment.
It's not a problem in this case since we use hardcoded chain tickers for NFTs
komodo-defi-framework/mm2src/coins/nft/nft_structs.rs
Lines 71 to 81 in 92372cb
| fn insert_tx_in_history_sql(chain: &Chain) -> MmResult<String, SqlError> { | ||
| let table_name = nft_tx_history_table_name(chain); | ||
| fn insert_transfer_in_history_sql(chain: &Chain) -> MmResult<String, SqlError> { | ||
| let table_name = nft_transfer_history_table_name(chain); | ||
| validate_table_name(&table_name)?; | ||
|
|
||
| let sql = format!( |
There was a problem hiding this comment.
This is a very dangerous way of building an sql statement and likely subject to sql injection. This goes for all the similarly built sql statements within this file.
There was a problem hiding this comment.
Oh my mistake, I misread this. Realizing now this is only inserting the table_name, not all the parameters.
There was a problem hiding this comment.
This function is just to build sql command string, which rusqlite actually asks to provide for execute() function for example.
It requires params sql: &str, params: P where in rusqlite, P: Params is a trait bound that indicates that the params argument must implement the Params trait. The Params trait is implemented for types that can be used as parameters in a prepared statement. In other words, P: Params means that the params variable must be of a type that can be used as parameters in the execute function. The Params trait is implemented for various types, including tuples, arrays, and slices of types that implement the ToSql trait.
execute() prepares sql statement, binds the parameters to the statement and execute it if you take a look at the source code. You can do it easily if you open project in Clion for example.
For params we create an array from Nft and NftTransferHistory fields
There was a problem hiding this comment.
as for table name, we validate it with validate_table_name function which does provide protection against sql injection attacks by only allowing alphanumeric characters and underscores in table names.
There was a problem hiding this comment.
Regarding the details_json field. I'm curious to know if it is justified to store both the individual fields and the entire JSON string containing those fields separately in the table. I'm not sure if there's a better way to do it, but is this approach architecturally justified?
There was a problem hiding this comment.
By the way, I am not very knowledgeable in SQL. However, I have noticed that we are currently using the COUNT(*) function to obtain the total number of entries in an SQLite table. We then use OFFSET and LIMIT to retrieve the desired page in the finalize_%something%_sql_builder function. This involves two separate queries. However, some tables have a log_index field which could potentially be used as a cursor. In other words, we can retrieve the desired page using the following query:
SELECT *
FROM table_name
WHERE log_index > %index_of_last_page_end%
ORDER BY log_index
LIMIT %page_size%;This would only require one query. It is possible that this approach may not be suitable in this case, or the number of entries in such tables may be small enough that there would be no significant difference between the COUNT(*) + OFFSET/LIMIT approach and the approach mentioned above. However, for large datasets, the approach described above would likely be more efficient.
Just thinking out loud, nothing more.
There was a problem hiding this comment.
Sorry, its a deep night, just a brief note, that log_index is an index of transaction, bcz there could be several transactions with the same transaction_hash, thats why we have primary key (transaction_hash, log_index), its not related to position in the sql table.
we count all items to show total objects in response. for example in get_nft_transfers
pub struct NftsTransferHistoryList {
pub(crate) transfer_history: Vec<NftTransferHistory>,
pub(crate) skipped: usize,
pub(crate) total: usize,
}There was a problem hiding this comment.
@DeckerSU to be more clear.
Unique transaction_hash can have only one transaction. A single transaction can trigger multiple events, and each event generates a log entry. For example, if a single transaction triggers three events, there will be three log entries associated with that transaction, each with a different log_index: 0, 1, and 2.
log_index as a primary key was added in this PR. If user sends a batch transfer (see safeBatchTransferFrom) with multiple NFTs you will see in response from moralis transactions with the same transaction_hash but different log_index for each NFT which was sent in this transaction.
There was a problem hiding this comment.
@laruh Thank you for the detailed explanation. I now understand that I misunderstood the purpose/function of the log_index field. I mistakenly thought it was used for indexing/ordering in the table, but now it is clear that it serves a different purpose. I apologize for taking up your time.
There was a problem hiding this comment.
@laruh Thank you for the detailed explanation. I now understand that I misunderstood the purpose/function of the
log_indexfield. I mistakenly thought it was used for indexing/ordering in the table, but now it is clear that it serves a different purpose. I apologize for taking up your time.
No no, it's alright, we are all here to understand and evaluate each other's code. Please feel free to ask anything!
Moreover, the question about details_json was important, I made a decision to try a bit another version of db tables, where we keep all unchangeable and unnecessary for select fields in details_json, while other fields will be kept in separate db fields (actually the same tables view as we have now, but another logic for details_json). This way should make the update some specific field process easier, bcz we dont need anymore to get rust structures from db first, update them and update db tables next.
Although it can affect get process bcz we have to fetch all fields from db to get rust structure, the code maintenance should get easier.
…ex (#1933) Global enabling of an account'/change/address_index path for all coins using hd_account_id config parameter is replaced by enable_hd which is a bool that defaults to false. path_to_address parameter is added to coins activation requests to set the default account'/change/address_index path that will be used for swaps. If not provided, the default will be 0'/0/0. HD withdrawal from any account'/change/address_index path is implemented for UTXO, EVM and Tendermint coins for now, other coins will be added later.
Binary report comparison from old to new mm2 builds shows reduction in file size after this commit. --------------------- Signed-off-by: onur-ozkan <[email protected]>
There was a problem hiding this comment.
LGTM from a security standpoint. However, the review did not cover the swap protocol upgrade (swap proto v2 changes) and watcher related components. I am not very familiar with the logic it should implement, so I cannot properly assess it from my perspective.
Improve ARRR synchronization based on a user-selected date. This feature will enable users to specify a specific date as the starting point for synchronization as a substitute for the checkpoint block from config or syncing from the first block. --------- Signed-off-by: borngraced <[email protected]>
Features:
Enhancements/Fixes: