feat(DVM) Add arbitrary data to price requests by chrismaree · Pull Request #2229 · UMAprotocol/protocol

chrismaree · 2020-11-27T15:11:35Z

Motivation

issue: #2234

This PR adds the ability for the DVM to accept any arbitrary data as part of a price request.

Summary

Briefly summarize what changes were made to accomplish the motivation above.

Details

This is done by accepting an bytes ancillaryData into the dvm price requests.

Issue(s)

Fixes 2234

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree · 2020-11-28T12:19:14Z

    { t: "int", v: request.salt },
    { t: "address", v: request.account },
    { t: "uint", v: request.time },
+    { t: "bytes", v: "0x" },


This change makes all downstream usages of EncryptionHelper work be default; with nothing other than this blank ancillary data.

For now, we will simply not support ancillary data in the voter dapp.

chrismaree · 2020-11-28T12:19:55Z

+    OracleAncillaryInterface,
+    VotingInterface,
+    VotingAncillaryInterface
+{


the Voting contract now considers the two new interfaces that define the Ancillary voting data.

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

…/protocol into chrismaree/arbitary-dvm-data

chrismaree · 2020-11-29T17:50:41Z


 async function decodeAllActiveGovernorProposals(artifacts, web3) {
  const Voting = artifacts.require("Voting");
-  const voting = await Voting.deployed();


This is basically the ripple change throughout the repo to force Truffle to use the overloaded methods (without the ancillary data prop).

To remove this we need to make all downstream tests and infrastructure consider only the ancillary methods and then we can desperate the overloaded method. This will enable us to roll back these test changes.

chrismaree · 2020-11-29T17:53:28Z

     * @return roundId defined as a function of the currentTime and `phaseLength` from `data`.
     */
    function computeCurrentRoundId(Data storage data, uint256 currentTime) internal view returns (uint256) {
-        uint256 roundLength = data.phaseLength.mul(uint256(VotingInterface.Phase.NUM_PHASES_PLACEHOLDER));


Unfortunately, some state had to move to the VotingAncillaryInterface to get the inheritance tree to compile correctly with truffle.

chrismaree · 2020-11-29T17:54:07Z

     * @param commits array of structs that encapsulate an `identifier`, `time`, `hash` and optional `encryptedVote`.
     */
-    function batchCommit(Commitment[] calldata commits) external virtual;
+    function batchCommit(Commitment[] memory commits) public virtual;


this needed to become public to enable calling internally for the override method.

chrismaree · 2020-11-29T17:55:13Z

   ***********************************/

  console.log("0. SETUP PHASE");
-  const voting = await Voting.deployed();


this was not added to the umip upgrade scripts clearly but is added to the vote related ones.

chrismaree · 2020-11-29T17:56:19Z

+  );
+}
+
+function computeVoteHashAncillary(request) {


however, I did add a method and tests to verify the behavior works as expected.

mrice32

@chrismaree, this is awesome! Big +1. Contracts/prod code all look great! I left a few pretty minor comments -- more as just mental notes for us to think about down the road.

Will move on to the tests, etc.

mrice32 · 2020-11-29T18:57:38Z

        // The index in the `pendingPriceRequests` that references this PriceRequest. A value of UINT_MAX means that
        // this PriceRequest is resolved and has been cleaned up from `pendingPriceRequests`.
        uint256 index;
+        bytes ancillaryData;


In some later version, we should probably think about ways to redesign our DVM to not rely on any of this being stored on chain, and instead only using an initial event emission + hash stored on chain.

I agree. ideally, emit the ancillaryData and just refer to a hash of it on-chain. This is a complexity trade-off that I think we leave for later though.

mrice32 · 2020-11-29T19:10:33Z

     * @param reveals array of the Reveal struct which contains an identifier, time, price and salt.
     */
-    function batchReveal(Reveal[] calldata reveals) external override {
+    function batchReveal(RevealAncillary[] memory reveals) public override {


Wow, I forgot about all of these methods.

mrice32 · 2020-11-29T19:16:15Z

+  );
+}
+
+function computeVoteHashAncillary(request) {


mrice32 · 2020-11-29T19:17:41Z

    { t: "int", v: request.salt },
    { t: "address", v: request.account },
    { t: "uint", v: request.time },
+    { t: "bytes", v: "0x" },


mrice32 · 2020-11-29T19:20:26Z

    { t: "int", v: request.salt },
    { t: "address", v: request.account },
    { t: "uint", v: request.time },
+    { t: "bytes", v: "0x" },


Do the following hashes match?

// pseudocode sha3(uint(5), uint(6)); sha3(uint(5), byes(0), uint(6));

If not, we need to be careful about releasing the common package as it will likely break compatibility with the current voting contract, right?

Update: they do seem to match (although someone else should verify :)). Very cool. In that case, is the "0x" element here basically just documentation to show that we're not leaving the value out?

yup, it shows we are leaving it out. it also mimics exactly how the hash is generated on-chain in revealVote(identifier, time, price, "", salt); with the blank string for the bytes type. When decoded this "" translates to 0x

It's really really nice that a 0 length data is perfectly backwards compatible when doing tight packing 👌

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

mrice32

Tests look awesome. A few very minor comments/nits

mrice32 · 2020-11-29T20:55:23Z

+    const identifier2 = web3.utils.utf8ToHex("request-retrieval2");
+    const time2 = "2000";
+    const ancillaryData2 = web3.utils.utf8ToHex(`callerAddress:${account4}`); // ancillary data should be able to store addresses


nit: I think it would make this test slightly better if we made the identifier and timestamp match between both requests and only have the data differ. Then, this test would verify that the DVM correctly differentiates.

Thoughts? I think it would only require us to set identifier2 and time2 to identifier1 and time1 on these lines.

good call. Have updated the tests accordingly and added a comment to explain why.

+1 -- thanks @chrismaree!

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

coveralls · 2020-11-29T22:13:14Z

Coverage increased (+0.1%) to 93.525% when pulling cdfba88 on chrismaree/arbitary-dvm-data into e6b63e5 on master.

chrismaree added 6 commits November 27, 2020 16:50

BROKEN WIP

e32fb46

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Updated implementation

4b1b5fa

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Updated encryption helper

afedd7d

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Added ripple changes

f211bec

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Merge branch 'master' into chrismaree/arbitary-dvm-data

4b04e45

Restore buidler.config.js

6f7c3f7

chrismaree commented Nov 28, 2020

View reviewed changes

chrismaree added 4 commits November 28, 2020 18:33

Updated methods and tests

03885db

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Updated encryption helper

cccd61d

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Added more comments

bcef818

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Merge branch 'chrismaree/arbitary-dvm-data' of github.com:UMAprotocol…

9c7f00c

…/protocol into chrismaree/arbitary-dvm-data

chrismaree commented Nov 29, 2020

View reviewed changes

mrice32 approved these changes Nov 29, 2020

View reviewed changes

chrismaree added 3 commits November 29, 2020 22:27

Updated tests

10ef634

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Merge branch 'master' into chrismaree/arbitary-dvm-data

b229e74

Updated nap spec

fb3a8d2

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree marked this pull request as ready for review November 29, 2020 20:36

mrice32 approved these changes Nov 29, 2020

View reviewed changes

Update packages/core/test/oracle/Voting.js

d336a0a

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree requested a review from nicholaspai November 29, 2020 21:03

Updated test for nit

cdfba88

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree merged commit 551ab99 into master Nov 29, 2020

chrismaree deleted the chrismaree/arbitary-dvm-data branch November 29, 2020 22:23

nicholaspai mentioned this pull request Apr 5, 2021

Add "1" Price identifier UMAprotocol/UMIPs#230

Closed

Conversation

chrismaree commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrismaree Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrismaree commented Nov 27, 2020 •

edited

Loading

mrice32 Nov 29, 2020 •

edited

Loading

chrismaree Nov 29, 2020 •

edited

Loading