Skip to content

Wrap transaction with new struct to include ISRs - #885

Merged
Manav-Aggarwal merged 12 commits into
mainfrom
manav/tx_serialize
May 5, 2023
Merged

Wrap transaction with new struct to include ISRs#885
Manav-Aggarwal merged 12 commits into
mainfrom
manav/tx_serialize

Conversation

@Manav-Aggarwal

@Manav-AggarwalManav-Aggarwal commented Apr 21, 2023

Copy link
Copy Markdown
Member

Closes: #880

@codecov

codecovBot commented Apr 21, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 0.40% and project coverage change: -1.32⚠️

Comparison is base (d7f6772) 55.91% compared to head (5b6eb32) 54.60%.

Additional details and impacted files
@@ Coverage Diff @@## main #885 +/- ##
==========================================
- Coverage 55.91% 54.60% -1.32% 
==========================================
Files 66 66 Lines 10709 10959 +250 ==========================================
- Hits 5988 5984 -4 - Misses 3849 4105 +256 + Partials 872 870 -2 
Impacted FilesCoverage Δ
types/tx.go51.72% <0.00%> (-48.28%)⬇️
types/pb/rollkit/rollkit.pb.go35.19% <0.42%> (-4.83%)⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Manav-AggarwalManav-Aggarwal self-assigned this Apr 21, 2023
@Manav-AggarwalManav-Aggarwal linked an issue Apr 21, 2023 that may be closed by this pull request
@Manav-AggarwalManav-Aggarwal changed the title Use sharesplitter to serialize/deserialize to and from sharesWrap transaction with new struct to include ISRsApr 21, 2023
@Manav-Aggarwal
Manav-Aggarwal marked this pull request as ready for review April 25, 2023 03:17
@gupadhyaya

Copy link
Copy Markdown
Contributor

@Manav-Aggarwal can you rebase with main again? looks like it is showing lots of changes from your previous PR.

@Manav-Aggarwal
Manav-Aggarwalforce-pushed the manav/tx_serialize branch 2 times, most recently from 8206833 to 7d77219CompareApril 25, 2023 15:38
@Manav-Aggarwal

Copy link
Copy Markdown
MemberAuthor

@Manav-Aggarwal can you rebase with main again? looks like it is showing lots of changes from your previous PR.

done

Comment threadproto/rollkit/rollkit.proto
Comment threadtypes/tx.go Outdated
Comment threadtypes/tx.go Outdated
Comment threadtypes/tx.go

@nashqueuenashqueue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you ! 1 nit 1 comment and a question.

Comment threadtypes/tx.go Outdated
Comment threadtypes/tx.go
Comment threadtypes/tx.go Outdated
@gupadhyaya

Copy link
Copy Markdown
Contributor

@Manav-Aggarwal

ignore the CI/CD data race failure, as i am working on resolving it.

for math/rand to crypto/rand, you can make the changes.

@Manav-Aggarwal

Copy link
Copy Markdown
MemberAuthor

@Manav-Aggarwal

ignore the CI/CD data race failure, as i am working on resolving it.

for math/rand to crypto/rand, you can make the changes.

Created issue for it here: #911

gupadhyaya
gupadhyaya previously approved these changes May 1, 2023

@nashqueuenashqueue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@Manav-Aggarwal
Manav-Aggarwal merged commit d6d030a into mainMay 5, 2023
@Manav-Aggarwal
Manav-Aggarwal deleted the manav/tx_serialize branch May 5, 2023 17:46
Sign up for freeto 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.

modify transaction to include ISR

3 participants

@Manav-Aggarwal@gupadhyaya@nashqueue