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

Remove Epoch from WrapperTx #2946

Merged

Conversation

cpp-phoenix
Copy link
Contributor

@cpp-phoenix cpp-phoenix commented Mar 22, 2024

Describe your changes

Closes #2691

Indicate on which release or other PRs this topic is based on

v0.34.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 59.39%. Comparing base (ea843f7) to head (1572234).
Report is 7 commits behind head on main.

❗ Current head 1572234 differs from pull request most recent head 90e6cb2. Consider uploading reports for the commit 90e6cb2 to get more accurate results

Files Patch % Lines
crates/light_sdk/src/transaction/pos.rs 0.00% 7 Missing ⚠️
crates/light_sdk/src/transaction/account.rs 0.00% 3 Missing ⚠️
crates/light_sdk/src/transaction/governance.rs 0.00% 2 Missing ⚠️
crates/light_sdk/src/transaction/pgf.rs 0.00% 2 Missing ⚠️
crates/sdk/src/tx.rs 0.00% 2 Missing ⚠️
crates/light_sdk/src/transaction/bridge.rs 0.00% 1 Missing ⚠️
crates/light_sdk/src/transaction/ibc.rs 0.00% 1 Missing ⚠️
crates/light_sdk/src/transaction/transfer.rs 0.00% 1 Missing ⚠️
crates/sdk/src/masp.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2946      +/-   ##
==========================================
- Coverage   59.41%   59.39%   -0.02%     
==========================================
  Files         298      298              
  Lines       92326    92690     +364     
==========================================
+ Hits        54853    55057     +204     
- Misses      37473    37633     +160     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@grarco grarco requested review from grarco and batconjurer April 11, 2024 10:21
Copy link
Member

@batconjurer batconjurer left a comment

Choose a reason for hiding this comment

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

Couple of small things, and it obviously needs a rebase. Otherwise looks good.

crates/light_sdk/src/transaction/mod.rs Outdated Show resolved Hide resolved
@@ -183,17 +183,15 @@ pub fn dump_tx<IO: Io>(io: &IO, args: &args::Tx, tx: Tx) {
/// to it.
#[allow(clippy::too_many_arguments)]
pub async fn prepare_tx<C: crate::queries::Client + Sync>(
client: &C,
_client: &C,
Copy link
Member

Choose a reason for hiding this comment

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

this argument should also be removed

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why this comment was marked as resolved. I would still like to have this argument removed. Especially since it removes the need for the generic, which should simplify other parts of the code base.

@Fraccaman Fraccaman mentioned this pull request Apr 15, 2024
@Fraccaman
Copy link
Member

@cpp-phoenix thanks for ur PR! Do u think u can think the issues in the code review and rebase on base ?

@brentstone brentstone force-pushed the cpp-phoenix/remove-epoch-from-wrappertx branch from d422dae to 246be23 Compare April 24, 2024 05:40
@brentstone brentstone requested a review from batconjurer April 24, 2024 05:42
@brentstone brentstone force-pushed the cpp-phoenix/remove-epoch-from-wrappertx branch from 246be23 to 1b80cff Compare April 24, 2024 06:03
@yito88
Copy link
Member

yito88 commented Apr 25, 2024

We need to update Hermes with this change. E2E tests should pass with https://github.com/heliaxdev/hermes/tree/1.7.4-namada-2946.

brentstone added a commit to cpp-phoenix/namada that referenced this pull request Apr 25, 2024
@@ -183,17 +183,15 @@ pub fn dump_tx<IO: Io>(io: &IO, args: &args::Tx, tx: Tx) {
/// to it.
#[allow(clippy::too_many_arguments)]
pub async fn prepare_tx<C: crate::queries::Client + Sync>(
client: &C,
_client: &C,
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why this comment was marked as resolved. I would still like to have this argument removed. Especially since it removes the need for the generic, which should simplify other parts of the code base.

@brentstone
Copy link
Collaborator

@batconjurer thx, I resolved the conversation because I thought I had remove the _client but somehow it slipped through again. Thanks for the note!

brentstone added a commit to cpp-phoenix/namada that referenced this pull request Apr 26, 2024
@brentstone brentstone force-pushed the cpp-phoenix/remove-epoch-from-wrappertx branch from 8f2565d to 05b2c93 Compare April 26, 2024 16:09
brentstone added a commit that referenced this pull request Apr 26, 2024
* cpp-phoenix/cpp-phoenix/remove-epoch-from-wrappertx:
  changelog: add #2946
  rebuild wasms for tests
  impl fix from review comment
  refactor: remove Epoch from WrapperTx
brentstone added a commit to cpp-phoenix/namada that referenced this pull request Apr 26, 2024
@brentstone brentstone force-pushed the cpp-phoenix/remove-epoch-from-wrappertx branch from 05b2c93 to 1572234 Compare April 26, 2024 17:18
brentstone added a commit that referenced this pull request Apr 26, 2024
…om-wrappertx' into brent/draft+2946+3100

* cpp-phoenix/cpp-phoenix/remove-epoch-from-wrappertx:
  changelog: add #2946
  rebuild wasms for tests
  impl fix from review comment
  refactor: remove Epoch from WrapperTx
brentstone added a commit that referenced this pull request Apr 26, 2024
* cpp-phoenix/cpp-phoenix/remove-epoch-from-wrappertx:
  changelog: add #2946
  rebuild wasms for tests
  impl fix from review comment
  refactor: remove Epoch from WrapperTx
brentstone added a commit that referenced this pull request Apr 26, 2024
* cpp-phoenix/cpp-phoenix/remove-epoch-from-wrappertx:
  changelog: add #2946
  rebuild wasms for tests
  impl fix from review comment
  refactor: remove Epoch from WrapperTx
@brentstone brentstone force-pushed the cpp-phoenix/remove-epoch-from-wrappertx branch from 1572234 to 7ac1bf3 Compare April 27, 2024 01:39
brentstone added a commit to cpp-phoenix/namada that referenced this pull request Apr 27, 2024
brentstone added a commit to cpp-phoenix/namada that referenced this pull request Apr 27, 2024
@brentstone brentstone force-pushed the cpp-phoenix/remove-epoch-from-wrappertx branch from 7ac1bf3 to e820bb5 Compare April 27, 2024 02:14
@brentstone brentstone force-pushed the cpp-phoenix/remove-epoch-from-wrappertx branch from e820bb5 to 90e6cb2 Compare April 27, 2024 02:18
brentstone added a commit that referenced this pull request May 8, 2024
* cpp-phoenix/cpp-phoenix/remove-epoch-from-wrappertx:
  changelog: add #2946
  rebuild wasms for tests
  impl fix from review comment
  refactor: remove Epoch from WrapperTx
@brentstone brentstone merged commit a59bf7d into anoma:main May 9, 2024
11 of 17 checks passed
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.

Remove Epoch from WrapperTx
6 participants