-
Notifications
You must be signed in to change notification settings - Fork 13
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
[On-Chain] Claim/Proof on-chain events #629
Conversation
The image is going to be pushed after the next commit. You can use If you also want to run E2E tests, please add |
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 629) |
dfe9c1f
to
3de0683
Compare
3de0683
to
7f8b902
Compare
0131b17
to
d48e73e
Compare
…tor+feat * pokt/main: [Relayminer] off-chain claim/proof message distribution (#619)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message MsgSubmitProofResponse { | ||
poktroll.proof.Proof proof = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
We should consider revisiting our Msg
keepers to return whatever they created/updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have events, that's a great event!
Left a few comments, this is probably the final review.
telemetry.ClaimComputeUnitsCounter( | ||
types.ClaimProofStage_CLAIMED, | ||
numComputeUnits, | ||
err, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we meter numRelays
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a telemetry call, not an event emission. This particular telemetry call is specific to compute units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm realizing that I misunderstood your message initially but I get it now. I'm not sure what the state of the code was at the time but now it looks like this:
telemetry.ClaimCounter(types.ClaimProofStage_CLAIMED, 1, err)
telemetry.ClaimComputeUnitsCounter(
types.ClaimProofStage_CLAIMED,
numComputeUnits,
err,
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Did it again. 😅🙈
I've opened #643 to track this.
event, err := cosmostypes.ParseTypedEvent(abci.Event(events[0])) | ||
require.NoError(t, err) | ||
|
||
claimCreatedEvent, ok := event.(*types.EventClaimCreated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smooth!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking here so preemptively approving.
Feel free to add TODOs if you think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks amazing!
return &types.MsgSubmitProofResponse{}, nil | ||
numRelays, err = claim.GetNumRelays() | ||
if err != nil { | ||
return nil, status.Error(codes.Internal, types.ErrProofInvalidClaimRootHash.Wrap(err.Error()).Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to reuse these errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe this error is accurate. If claim.GetNumRelays()
or claim.GetNumComputeUnits()
fails, it's because the root is invalid.
|
||
option go_package = "github.com/pokt-network/poktroll/x/proof/types"; | ||
|
||
enum ClaimProofStage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this
message MsgSubmitProofResponse { | ||
poktroll.proof.Proof proof = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
@@ -297,3 +250,71 @@ func (s *suite) waitForNewBlockEvent( | |||
s.Log("Success; message detected before timeout.") | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These helpers LGTM. Did you take some time to make sure we reuse them elsewhere? (i.e. some of the work I did that should leverage them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yessir! 🫡 My observation was that the spaghetti code was strewn between the session, relay, and settlement scenarios.
Co-authored-by: Redouane Lakrache <[email protected]>
Summary
num_relays
and renamecompute_units
tonum_compute_units
.Issue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start
; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_test
make test_e2e
PR Testing (only if making code changes)
devnet-test-e2e
label to the PR.make trigger_ci
if you want to re-trigger tests without any code changesSanity Checklist