Skip to content

Comments

chore: Update to replica v0.5.4#552

Merged
pikajude merged 5 commits intomasterfrom
paulliu/update-to-replica-v0.5.4
Apr 8, 2020
Merged

chore: Update to replica v0.5.4#552
pikajude merged 5 commits intomasterfrom
paulliu/update-to-replica-v0.5.4

Conversation

@ninegua
Copy link
Member

@ninegua ninegua commented Apr 7, 2020

@pikajude noticed some problem, so I'm trying to see if they can be fixed.

@ninegua ninegua requested a review from a team April 7, 2020 23:47
@ninegua ninegua requested a review from a team as a code owner April 7, 2020 23:47
@ninegua ninegua changed the title Update to replica v0.5.4 chore: Update to replica v0.5.4 Apr 7, 2020
Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

LGTM, assuming it passes CI

@ninegua
Copy link
Member Author

ninegua commented Apr 8, 2020

@hansl No, it does not pass CI. Somehow dfx submits SignedMessage to replica, and I don't think it is understood yet. Turns out it was the content field being no longer flattened.

@ninegua ninegua force-pushed the paulliu/update-to-replica-v0.5.4 branch from ccd8bdf to d19e48f Compare April 8, 2020 00:50
@ninegua ninegua force-pushed the paulliu/update-to-replica-v0.5.4 branch from c2b66fd to 7d1e836 Compare April 8, 2020 05:26
@ninegua
Copy link
Member Author

ninegua commented Apr 8, 2020

Having resolved the problems, I think it is basically unmaintainable not to share a crate for http request/response data types between sdk and replica. What is the plan going forward?

@ninegua ninegua force-pushed the paulliu/update-to-replica-v0.5.4 branch 2 times, most recently from 43372b8 to 95c4790 Compare April 8, 2020 08:13
@ninegua ninegua requested review from eftychis and hansl April 8, 2020 09:16
}
#[derive(Debug, Deserialize, PartialEq, Eq)]
#[serde(untagged)]
pub enum Replied {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the difference is between Option<Blob> and this data type (as far as serde is concerned they encode to the same data).

Copy link
Member Author

@ninegua ninegua Apr 8, 2020

Choose a reason for hiding this comment

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

It was copied from http_handler, and I was not familiar with the motivation of changing it. However, it is an enum with flipped order (Empty comes last, where in Option type None comes first), so it does not decode to the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

They encode to the same interface { arg?: Blob }. Making arg an Option<Blob> results in the same encoding (which is what we had before this PR).

My concern is that this PR touches more than the minimum to make it work.

@pikajude pikajude force-pushed the paulliu/update-to-replica-v0.5.4 branch from 95c4790 to 63e4c8d Compare April 8, 2020 17:46
@ninegua ninegua force-pushed the paulliu/update-to-replica-v0.5.4 branch from 63e4c8d to 75c471c Compare April 8, 2020 17:48
@pikajude pikajude merged commit 89e7669 into master Apr 8, 2020
@chenyan-dfinity
Copy link
Contributor

This PR breaks userlib.

POST http://127.0.0.1:8000/api/v1/read 422 (Unprocessable Entity)
read @ index.js:formatted:7889 
async function (async) 
read @ index.js:formatted:7887 
query @ index.js:formatted:7909 
retrieveAsset @ index.js:formatted:7918 
_loadJs @ index.js:formatted:4907 
_main

@ninegua
Copy link
Member Author

ninegua commented Apr 9, 2020

Very likely userlib will have to adapt to the request/response data type changes from http_handler. How do I reproduce the error you had? @chenyan-dfinity

@chenyan-dfinity
Copy link
Contributor

Likely. You can run dfx start on this branch. Then build and install the default canister. Visit http://127.0.0.1:8000/?canisterId=ic:XX will see the error message.

@lwshang lwshang deleted the paulliu/update-to-replica-v0.5.4 branch July 29, 2022 19:00
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.

6 participants