-
Notifications
You must be signed in to change notification settings - Fork 32
[WIP] feat(pdp): support create and upload #692
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
base: pdpv0
Are you sure you want to change the base?
Conversation
Signed-off-by: Jakub Sztandera <[email protected]>
Signed-off-by: Jakub Sztandera <[email protected]>
64afc4a
to
726cf4d
Compare
return fmt.Errorf("expeted to find dataSetId in receipt but failed to extract: %w", err) | ||
} | ||
// XXX: I considered here chekcing if dataset exists already in DB, but not sure if it is needed | ||
} |
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.
The majority of my questions are around this file. I'm yet to test it, but I'm using dataset=0 as a sentinel value.
It might be better if it were maybe NULL. Also, I don't know about any possible table relations that this might affect.
@LexLuthr
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 think you should look at handleGetPieceAdditionStatus, that's the only place I can see where it might matter - client can ask for piece addition status and it selects by data set id; what should the client ask for in the case of the combined flow and what do we expect them to get, because I think maybe none of it works currently. We call that with getPieceAdditionStatus
in the SDK.
The only other place where we have functionality outside of this PR that impacts that table is the trigger we add for transaction resolving and it doesn't care about data set id:
curio/harmony/harmonydb/sql/20250730-pdp-rename.sql
Lines 155 to 170 in 435f7b8
CREATE OR REPLACE FUNCTION update_pdp_data_set_piece_adds() | |
RETURNS TRIGGER AS $$ | |
BEGIN | |
IF OLD.tx_status = 'pending' AND (NEW.tx_status = 'confirmed' OR NEW.tx_status = 'failed') THEN | |
-- Update the add_message_ok field in pdp_data_set_piece_adds if a matching entry exists | |
UPDATE pdp_data_set_piece_adds | |
SET add_message_ok = CASE | |
WHEN NEW.tx_status = 'failed' OR NEW.tx_success = FALSE THEN FALSE | |
WHEN NEW.tx_status = 'confirmed' AND NEW.tx_success = TRUE THEN TRUE | |
ELSE add_message_ok | |
END | |
WHERE add_message_hash = NEW.signed_tx_hash; | |
END IF; | |
RETURN NEW; | |
END; | |
$$ LANGUAGE plpgsql; |
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 could special-case dataset=0 in handleGetPieceAdditionStatus
Signed-off-by: Jakub Sztandera <[email protected]>
@rvagg would appriciate you taking a look as well. |
// POST /pdp/data-sets - Create a new data set | ||
r.Post("/", p.handleCreateDataSet) | ||
|
||
// POST /pdp/data-sets/create-and-add - Create a new data set and add pices at the same time |
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.
// POST /pdp/data-sets/create-and-add - Create a new data set and add pices at the same time | |
// POST /pdp/data-sets/create-and-add - Create a new data set and add pieces at the same time |
|
||
type RequestBody struct { | ||
RecordKeeper string `json:"recordKeeper"` | ||
Pieces []AddPieceRequest `json:"pieces"` |
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.
oh yeah, nicely typed
return false, err | ||
} | ||
// XXX: The zero (0) here is a hack, it would be better if this was nullable. Not sure if this will work | ||
err = p.insertPieceAdds(tx, 0, txHashLower, reqBody.Pieces, subPieceInfoMap) |
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.
important lint failure
if err != nil { | ||
return false, err | ||
} | ||
// XXX: The zero (0) here is a hack, it would be better if this was nullable. Not sure if this will work |
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.
you could just make it nullable then, it's not hard to put in a migration for the table
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.
but I don't mind 0
here
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 realising this file should have been renamed long ago, you could rename it if you're inclined as long as you're in here s/root/pieces
Looks good, my only concern is the impact on |
No description provided.