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.

Sign up for GitHub

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

Jump to bottom

Implement starknet_getMessageStatus #2184

Merged
merged 38 commits into from
Nov 19, 2024
Merged

Implement starknet_getMessageStatus #2184

merged 38 commits into from
Nov 19, 2024

Conversation

rianhughes
Copy link
Contributor

closes #2178

@codecov Codecov
Copy link

codecov bot commented Oct 1, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 68.30601% with 58 lines in your changes missing coverage. Please review.

Project coverage is 75.29%. Comparing base (226c571) to head (c7dfcb9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
rpc/l1.go 72.72% 12 Missing and 6 partials ⚠️
migration/migration.go 74.00% 9 Missing and 4 partials ⚠️
blockchain/blockchain.go 74.35% 7 Missing and 3 partials ⚠️
l1/eth_subscriber.go 10.00% 9 Missing ⚠️
rpc/handlers.go 75.00% 2 Missing and 1 partial ⚠️
l1/l1.go 0.00% 2 Missing ⚠️
rpc/transaction.go 0.00% 2 Missing ⚠️
node/node.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2184      +/-   ##
==========================================
- Coverage   75.40%   75.29%   -0.11%     
==========================================
  Files         106      107       +1     
  Lines       11237    11392     +155     
==========================================
+ Hits         8473     8578     +105     
- Misses       2128     2166      +38     
- Partials      636      648      +12

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

🚨 Try these New Features:

@rianhughes rianhughes force-pushed the rianhughes/rpc8-getMessageStatus2 branch from 235811c to 7322767 Compare October 3, 2024 08:11
@rianhughes rianhughes marked this pull request as ready for review October 3, 2024 08:44
@rianhughes rianhughes requested a review from kirugan October 3, 2024 08:44
@rianhughes rianhughes force-pushed the rianhughes/rpc8-getMessageStatus2 branch from d31039e to 4f6fd6d Compare October 3, 2024 08:45
pnowosie
pnowosie approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@pnowosie pnowosie left a comment

Choose a reason for hiding this comment

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

Just one question

blockchain/blockchain.go Outdated Show resolved Hide resolved
kirugan
kirugan reviewed Oct 8, 2024
View reviewed changes
db/buckets.go Outdated Show resolved Hide resolved
@rianhughes rianhughes marked this pull request as draft October 9, 2024 08:14
@rianhughes rianhughes marked this pull request as ready for review October 9, 2024 08:29
@rianhughes rianhughes requested a review from kirugan October 9, 2024 08:30
kirugan
kirugan reviewed Oct 10, 2024
View reviewed changes
blockchain/blockchain.go Outdated Show resolved Hide resolved
kirugan
kirugan reviewed Oct 10, 2024
View reviewed changes
blockchain/blockchain.go Outdated Show resolved Hide resolved
kirugan
kirugan reviewed Oct 10, 2024
View reviewed changes
migration/migration.go Outdated Show resolved Hide resolved
@rianhughes rianhughes force-pushed the rianhughes/rpc8-getMessageStatus2 branch 2 times, most recently from 8ebd0b7 to 8020f6a Compare October 23, 2024 07:51
kirugan
kirugan reviewed Oct 31, 2024
View reviewed changes
blockchain/blockchain.go Outdated Show resolved Hide resolved
kirugan
kirugan reviewed Oct 31, 2024
View reviewed changes
grpc/gen/kv.pb.go Outdated Show resolved Hide resolved
kirugan
kirugan reviewed Oct 31, 2024
View reviewed changes
node/node.go Outdated Show resolved Hide resolved
kirugan
kirugan reviewed Oct 31, 2024
View reviewed changes
rpc/l1.go Show resolved Hide resolved
kirugan
kirugan reviewed Oct 31, 2024
View reviewed changes
rpc/l1.go Outdated Show resolved Hide resolved
IronGauntlets
IronGauntlets reviewed Oct 31, 2024
View reviewed changes
rpc/l1.go Outdated Show resolved Hide resolved
@rianhughes rianhughes force-pushed the rianhughes/rpc8-getMessageStatus2 branch from 3d9de48 to 907ce49 Compare November 1, 2024 13:04
@rianhughes rianhughes force-pushed the rianhughes/rpc8-getMessageStatus2 branch from ff047a5 to e16cab2 Compare November 4, 2024 13:47
IronGauntlets
IronGauntlets requested changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@IronGauntlets IronGauntlets left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think there is some repetition around the ethereum client which can be removed.

With regards to migration, it is going to be quite large (@kirugan) and since this has to do with Hashing, it can potentially take quite some time (I would expect it to take several hours) but it will be faster than resyncing the whole chain.

I would prefer to sync from scratch and create a new snapshot so we can avoid adding this migration (if we have time). The rpc may be down if this is not planned well, as an upgrade would block Juno until the migration is complete which cannot stopped.

blockchain/blockchain.go Show resolved Hide resolved
blockchain/blockchain.go Outdated Show resolved Hide resolved
blockchain/blockchain_test.go Outdated Show resolved Hide resolved
blockchain/blockchain_test.go Outdated Show resolved Hide resolved
blockchain/blockchain_test.go Outdated Show resolved Hide resolved
rpc/l1.go Outdated Show resolved Hide resolved
l1/eth_subscriber.go Outdated Show resolved Hide resolved
node/node.go Outdated Show resolved Hide resolved
rpc/handlers.go Outdated Show resolved Hide resolved
rpc/l1.go Outdated Show resolved Hide resolved
rianhughes and others added 14 commits November 7, 2024 13:50
@rianhughes @IronGauntlets
Update rpc/l1.go
b9d107c 
Co-authored-by: Aneeque <[email protected]>
Signed-off-by: Rian Hughes <[email protected]>
@rianhughes
update tests and make Hash func private
226c576
@rianhughes
reduce repitition in node.go
148187c
@rianhughes
to squash
f3a09f6
@rianhughes
remove EthClient
fb08240
@rianhughes
Merge branch 'main' into rianhughes/rpc8-getMessageStatus2
60d072e
@rianhughes
update the l1HandlerTxnHashByMsgHash
8b717e2
@IronGauntlets
Refactor l1HandlerTxnHash test and get fn
bc2d101
@IronGauntlets
Remove misleading todo
86c6384
@IronGauntlets
Add L1() to l1Client to return the l1 subscriber
fba5243
@IronGauntlets
Add l1Client interface in rpc package
9612e7a
@IronGauntlets
Remove unnecessary warning
ffd8b33 
The user chooses not to provide eth node therefore they are expected to
understand the consequences. Also, there is already an error message is
someone tries to request `starknet_getMessageStatus` without providing
an eth-node.
@IronGauntlets
Make logMessageToL2 private
cb8e1cb
@IronGauntlets
Remove ErrL1ClientNotFound variable
810a6bd 
Since it is only used in one place there is no need to have a variable
for it.
@rianhughes
Copy link
Contributor Author

The above changes LGTM

@kirugan kirugan merged commit 7286fe7 into main Nov 19, 2024
13 checks passed
@kirugan kirugan deleted the rianhughes/rpc8-getMessageStatus2 branch November 19, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IronGauntlets IronGauntlets IronGauntlets approved these changes

@pnowosie pnowosie pnowosie approved these changes

@kirugan kirugan kirugan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RPC08 - starknet_getMessagesStatus
4 participants
@rianhughes @kirugan @pnowosie @IronGauntlets