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

chore(release): v1.0.5-beta #1844

Merged
merged 9 commits into from
Jun 8, 2023
Merged

chore(release): v1.0.5-beta #1844

merged 9 commits into from
Jun 8, 2023

Conversation

shamardy
Copy link
Collaborator

@shamardy shamardy commented May 29, 2023

Features:

  • NFT integration #900
    • UriMeta was added to get info from token uri, status and metadata in nft tx history #1823

Enhancements/Fixes:

  • Deprecated wasm-timer dependency was removed from mm2 tree #1836
  • log, getrandom and wasm-bindgen dependencies were updated to more recent versions that are inline with the latest libp2p upstream #1837
  • A CI lint pipeline was added that validates pull request titles to ensure that they comply with the conventional commit specifications #1839
  • KMD AUR were reduced from 5% to 0.01% starting at nS7HardforkHeight to comply with KIP-0001 #1841

onur-ozkan and others added 5 commits May 24, 2023 20:06
… v0.1.12 (#1836)

This partially excludes the deprecated dependency `wasm-timer` from mm2 tree. `wasm-timer` is still used but only in the p2p stack from libp2p which will also be removed with #1756
…ransferHistory (#1823)

This commit introduces a new struct called UriMeta that covers the necessary info about a token: image, token_name, description, attributes, and animation_url. It also adds new fields to NftTransferHistory such as collection_name and status. The collection_name field shows the name of the collection that the token belongs to. The status field indicates whether the transfer status is Receive or Send.
…ent versions (#1837)

This commit updates the log, getrandom and wasm-bindgen dependencies to more recent versions that are inline with the latest libp2p upstream.

Signed-off-by: ozkanonur <work@onurozkan.dev>
Adds a CI step that validates the pull request title to ensure that it complies with the conventional commit specifications. A pipeline will be triggered upon opening a pull request and upon renaming the pull request title.

Signed-off-by: ozkanonur <work@onurozkan.dev>
…ght (#1841)

KMD interest calculation is adjusted to reduce AUR (Active User Rewards) from 5% to 0.01% starting from KMD block height `3484958` (Fri Jun 30 2023) according to [KIP-0001](https://github.com/KomodoPlatform/kips/blob/main/kip-0001.mediawiki)

fixes #1840
@shamardy shamardy changed the title [release] v1.0.5-beta chore(release): v1.0.5-beta May 29, 2023
@shamardy shamardy added the blocked requires additional attention label May 30, 2023
@shamardy
Copy link
Collaborator Author

Needs this #1845 to be ready

@shamardy shamardy removed the blocked requires additional attention label May 30, 2023
minutes -= 59;
let accrued = (value / 10_512_000) * minutes;
let mut accrued = (value as f64 * AUR_PER_MINUTE) as u64 * minutes;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm just curious why we need to use floating point arithmetic here, convert from integer to float and back, use drop_mutability! macros, rather than using something simpler like:

    let accrued = if height >= N_S7_HARDFORK_HEIGHT {
        (value / 10_512_000) * minutes / 500
    } else {
        (value / 10_512_000) * minutes
    };

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your feedback. The code you provided is better than using drop_mutability!, I will fix it in a final fixes PR that I will open.

As for using floating point arithmetic, it was to make the code more understandable as suggested here #1841 (comment) , I think I will also go with your suggestion to avoid multiple type castings, I can write a comment above the code to make it more understandable. What do you think @caglaryucekaya since the previous change was your suggestion?

Choose a reason for hiding this comment

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

I agree that conversion from integer to float is not necessary, but I think using constants is better than having magic numbers around the code. So I suggest the following:

// MINUTES_PER_YEAR = 365 * 24 * 60
const MINUTES_PER_YEAR: u64 = 525_600;
// Minutes required for 100% active user reward before N_S7_HARDFORK_HEIGHT
const MINUTES_PER_AUR: u64 = 20 * MINUTES_PER_YEAR;
    
let accrued = if height >= N_S7_HARDFORK_HEIGHT {
    (value / MINUTES_PER_AUR) * minutes / 500
} else {
    (value / MINUTES_PER_AUR) * minutes
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

* remove nft and history len checks

* transfer_list.len() > 0 check

* use is_empty for list
@@ -111,7 +111,7 @@ version = "0.7.6"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "fcb51a0695d8f838b1ee009b3fbf66bda078cd64590202a864a8f3e8c4315c47"
dependencies = [
"getrandom 0.2.6",
"getrandom 0.2.9",
Copy link

Choose a reason for hiding this comment

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

Comment on lines +228 to +229
"proc-macro2 1.0.58",
"quote 1.0.27",
Copy link

Choose a reason for hiding this comment

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

@DeckerSU plz cross-validate (secure code review)

@@ -90,14 +90,14 @@ sp-trie = { version = "6.0", default-features = false }
trie-db = { version = "0.23.1", default-features = false }
trie-root = "0.16.0"
uuid = { version = "1.2.2", features = ["fast-rng", "serde", "v4"] }
wasm-timer = "0.2.4"
instant = { version = "0.1.12" }
Copy link

Choose a reason for hiding this comment

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

minor feedback: this one doesn't look maintained and relies on stdweb which isn't maintained neither. i.e we might want to consider using an alternative or fork it over and useweb-sys instead of stdweb for better future compatibility as its actively maintained and generally recommended for new projects targeting WASM support.

Copy link
Member

Choose a reason for hiding this comment

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

wasm-timer is deprecated and we can remove it from our dependency tree (it's only used in our libp2p fork, which will be removed once we switch to upstream).

But removing instant is not very possible to do. Lots of core dependencies already depending on it. We basically removed wasm-timer from dependency tree and used instant without adding anything to dependency tree.

@@ -18,6 +18,6 @@ parking_lot = { version = "0.12.0", features = ["nightly"] }
serde = "1.0"
serde_json = { version = "1", features = ["preserve_order", "raw_value"] }
serde_derive = "1.0"
wasm-bindgen = { version = "0.2.50", features = ["nightly"] }
wasm-bindgen = "0.2.86"
Copy link

Choose a reason for hiding this comment

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

rustwasm/wasm-bindgen@0.2.50...0.2.86 is quite massive and will require additional time. cc @DeckerSU @Alrighttt will need help - also might make sense to split it up between us and continue the complete (other 2/3) review AFTER co-approval.

Copy link

Choose a reason for hiding this comment

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

cc @shamardy jfyi ^

@@ -198,7 +198,7 @@ dependencies = [
"futures-io",
"futures-timer",
"kv-log-macro",
"log 0.4.14",
"log 0.4.17",
Copy link

Choose a reason for hiding this comment

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

reviewed

Copy link
Member

@Alrighttt Alrighttt left a comment

Choose a reason for hiding this comment

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

https://diff.rs/getrandom/0.2.0/0.2.9/
https://diff.rs/log/0.4.8/0.4.17/
https://diff.rs/quote/1.0.18/1.0.27/
https://diff.rs/proc-macro2/1.0.39/1.0.58
https://diff.rs/wasm-bindgen-shared/0.2.78/0.2.86/
instant 0.1.12

Reviewed the above and all changes.

There are two additional updated dependencies with very large diffs, https://diff.rs/syn/1.0.95/2.0.16/ and https://diff.rs/wasm-bindgen/0.2.50/0.2.86/ .

I am currently making my way through wasm-bindgen.

ca333
ca333 previously approved these changes Jun 6, 2023
Copy link

@ca333 ca333 left a comment

Choose a reason for hiding this comment

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

sec reviewed adex contributions @ 3b9aa92e00c66ddddc859905cf92b4984d346f9b
sec reviewed deps via local clone and ran comparison/integrity check against .cargo/registry:

rust-random/getrandom@v0.2.6...v0.2.9
dtolnay/proc-macro2@1.0.37...1.0.58
dtolnay/quote@1.0.18...1.0.27
https://github.com/sebcrozet/instant/tree/v0.1.12
rust-lang/log@0.4.14...0.4.17
dtolnay/syn@1.0.95...2.0.16

rustwasm/wasm-bindgen@0.2.50...0.2.86 [WIP]

@Alrighttt Alrighttt self-requested a review June 7, 2023 16:28
Alrighttt
Alrighttt previously approved these changes Jun 7, 2023
Copy link
Member

@Alrighttt Alrighttt left a comment

Choose a reason for hiding this comment

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

Approval conditional on @ca333's approval of syn changes.

@shamardy
Copy link
Collaborator Author

shamardy commented Jun 8, 2023

Approval conditional on @ca333's approval of syn changes.

I believe syn was approved here #1844 (review).

Will update the release date in the changelog then merge this PR.

@shamardy shamardy dismissed stale reviews from Alrighttt and ca333 via b48d73e June 8, 2023 17:46
@shamardy shamardy merged commit 1d8bebd into main Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants